Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String rewrite system based definition of offset normalization #9

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ehatti
Copy link

@ehatti ehatti commented Jan 20, 2025

Redefines the definition of pointer offset normalization in this file as a string rewrite system. Current progress: The pointer equalities in the PTRS interface are all provable, vaddr and alloc_id axioms have not been proven -- those are orthogonal to the definition of normalization however.

roff_rw_local defines the local rewrites, namely that o_base and o_derived cancel out, that indexing the 0th element is a no-op, and that sequences o_subs should be merged together. roff_rw_global lifts this to a rewrite relation on entire offsets, and finally roff_rw takes the closure of this to define the rewrite system.

From there we define a function normalize -- which is used to ensure the result of offset composition is still normalized -- and prove it is equivalent to roff_rw. I've proven that it is sound wrt roff_rw, but I still need to prove the opposite direction.

I'm using the Equations library to get nicer well-founded recursion, since Program Fixpoint generates quite unworkable terms. Let me know if adding this extra dependency is a problem, if it is then we can switch to a different definition for normalize.
Equations is just nice because it "just works" and so makes it faster to get the idea into Coq.

Copy link
Contributor

@gmalecha gmalecha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks really great. These are mostly small, stylistic comments.

*)

(**
Another (incomplete) consistency proof for [PTRS], based on Krebbers' PhD thesis, and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should revise this text.

Require Import bedrock.lang.cpp.model.simple_pointers_utils.
Require Import bedrock.lang.cpp.model.inductive_pointers_utils.

From Equations Require Import Equations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should move up towards the top of the file since it is a Rocq library.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we generally seem to avoid From...Require

Module PTRS_IMPL <: PTRS_INTF.
Import canonical_tu address_sums merge_elems.

Inductive raw_offset_seg : Set :=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose adding a comment here or somewhere to outline the setup. Maybe we can put all of the non-canonical stuff in a module like:

Module raw_offset.

End raw_offset.

Definition offset := { o : raw_offset.t | raw_offset.is_canonical o }.

And then hide most of the non-canonical stuff.


From Equations Require Import Equations.

Axiom irr : ∀ (P : Prop) (p q : P), p = q.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not ultimately necessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not strictly necessary, but it does make it much easier to prove equality of offsets (since we don't have to prove equalities of proofs).

Proof. solve_decision. Defined.
#[global] Declare Instance raw_offset_seg_countable : Countable raw_offset_seg.

Definition offset_seg : Set := raw_offset_seg * Z.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Blaisorblade @ehatti I forget, what is the motivation for including the Z here? Can it be removed? It seems like it is always computed from the other component (given a genv) so maybe this naturally comes in at another layer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ehatti I think this is the main technical comment, so I wanted to highlight it here.

Module PTRS_IMPL <: PTRS_INTF.
Import canonical_tu address_sums merge_elems.

Inductive raw_offset_seg : Set :=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Inductive raw_offset_seg : Set :=
Variant raw_offset_seg : Set :=

Comment on lines 623 to 627
Lemma rw_bwd_r :
∀ o1 o2 o2' o3,
roff_rw o2 o2' ->
roff_rw (o1 ++ o2) o3 ->
roff_rw (o1 ++ o2') o3.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lemma vaguely looks like a Proper instance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a Proper instance for the function composition fun o => roff_rw (o1 ++ o) o3 — this isn't really applicable. But roff_rw_cong can be a Proper instance, then rw_bwd_r would be proved by move => o1 o2 o2' o3 ->., and I'd suggest deleting it entirely and using rewrite at the call site.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll go ahead and make it a Proper instance

by destruct H3.
Qed.

Inductive root_ptr : Set :=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Inductive root_ptr : Set :=
Variant root_ptr : Set :=

| offset_ptr (p : root_ptr) (o : offset).
Definition ptr := ptr_.

Program Definition __offset_ptr (p : ptr) (o : offset) : ptr :=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Program Definition __offset_ptr (p : ptr) (o : offset) : ptr :=
#[program] Definition __offset_ptr (p : ptr) (o : offset) : ptr :=

| fun_ptr_ (tu : translation_unit_canon) (o : obj_name)
| alloc_ptr_ (a : alloc_id).

Inductive ptr_ : Set :=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Inductive ptr_ : Set :=
Variant ptr_ : Set :=

Comment on lines +139 to +143
Lemma roff_rw_cong :
∀ ol1 ol2 or1 or2,
roff_rw ol1 or1 ->
roff_rw ol2 or2 ->
roff_rw (ol1 ++ ol2) (or1 ++ or2).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be roughly:

Suggested change
Lemma roff_rw_cong :
∀ ol1 ol2 or1 or2,
roff_rw ol1 or1 ->
roff_rw ol2 or2 ->
roff_rw (ol1 ++ ol2) (or1 ++ or2).
#[global] Instance roff_rw_cong :
Proper (roff_rw ==> roff_rw ==> roff_rw) (++).

roff_rw (o1 ++ o2) o3 ->
roff_rw (o1' ++ o2) o3.
Proof.
Admitted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best delete, like rw_bwd_r.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are actually needed, they essentially states that normalization is order-independent, which is a crucial property as it's what allows the proofs of the offset equalities to be very easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants