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

Cxx/lax memory #294

Merged
merged 3 commits into from
Aug 8, 2019
Merged

Cxx/lax memory #294

merged 3 commits into from
Aug 8, 2019

Conversation

robdockins
Copy link
Contributor

This PR relaxes some of the rules regarding when pointers can be compared in ways that are disallowed by the C standard, but are nonetheless common and appear in libcxx.

This PR exists as a reminder to

  1. Setup up configuration knobs for being able to selectively enable this, and similar, relaxations.
  2. Investigate if we can/should equip pointers with a more principled total order by e.g., abstracting over all possible allocation block orderings using an arbitrary permutation.

regarding pointer identity to be relaxed.  In particular, there
is a flag to allow order comparisons on arbitrary pointers (i.e.,
not from the same allocation block) and for equality comaprisons on
pointers to constant data.
@robdockins
Copy link
Contributor Author

I think this is ready to go in now. The two pointer relaxations this branch considers are now controlled by a pair of boolean flags in a MemOptions record, which must be passed in when setting up the simulator.

Copy link
Member

@kquick kquick left a comment

Choose a reason for hiding this comment

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

Looks good, just one side question inline with the code. Thanks!

llvmExtensionImpl =
llvmExtensionImpl :: (HasPtrWidth (ArchWidth arch)) => MemOptions -> ExtensionImpl p sym (LLVM arch)
llvmExtensionImpl mo =
let ?memOpts = mo in
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming that this is an implicit param to avoid a much more intrusive implementation that would have threaded this explicitly through lots of places to make it available where it was needed? If so, you might want to mention that in the PR description for posterity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... at the moment it doesn't make much difference, but if similar options become more prevalent, I expect it will be a win.

Copy link
Contributor

@langston-barrett langston-barrett left a comment

Choose a reason for hiding this comment

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

A more general solution would be allowing clients of the memory model to construct a filter function

UndefinedBehavior e -> Bool

and only using that in MemModel.assertUndefined to only make assertions that the client cares about. That being said, we can consider that at another time if this is easier for now.

@kquick
Copy link
Member

kquick commented Aug 8, 2019

Although deferring the control to the client is a nice option, we would still need to have some sort of indicator for what was being attempted to allow the client to decide what type of undefined operation was being attempted and at this stage the only real options are "fail" or "continue", so I don't think it would simplify the implementation very much. Like you said though, we could transition to that later on if it turns out to be useful.

@robdockins
Copy link
Contributor Author

I'd be open to that kind of refactoring at some point, but I don't think it would simplify things much now. Also, we actually change the definition of pointer comparison depending on the flag, so a system for filtering UB isn't quite enough.

@langston-barrett
Copy link
Contributor

@kquick I don't understand your comment. I'm suggesting a similar mechanism to what is in this MR, the difference would be allowing clients to enable or disable checking for any kind of undefined behavior, whereas this MR addresses two specific instances.

@robdockins Doesn't the "lax" definition of pointer equality subsume the default one? It seems to me we just need to perform the lax check and toggle whether the assertion about the base pointer equality is made.

@kquick
Copy link
Member

kquick commented Aug 8, 2019

I was suggesting that we would need to define various UndefinedBehavior instances (I'm assuming e is a parameter over the language) and my assumption is that the net changes for the current needs would probably be similar or possibly more for that approach. I like the aspect of your proposal adding more ability for the client to control things, I'm just being conservative and agree that those types of generalizations could be a future direction. I'll come by to chat though in case I've misunderstood.

@langston-barrett
Copy link
Contributor

@kquick Thanks for clarifying!

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