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

macaw-symbolic-syntax: Concrete syntax for macaw-symbolic CFGs #345

Merged
merged 10 commits into from
Nov 3, 2023

Conversation

langston-barrett
Copy link
Contributor

@langston-barrett langston-barrett commented Nov 1, 2023

Fixes #344. Will require merging and bumping the Crucible submodule to bring in GaloisInc/crucible#1113.

This code was ported from ambient-verifier.
@langston-barrett langston-barrett marked this pull request as ready for review November 2, 2023 13:11
@langston-barrett
Copy link
Contributor Author

From a conversation with @RyanGlScott:

  • The Pointer type now exists in crucible-llvm-syntax and macaw-symbolic-syntax. We should try to unify the surface syntax so that at least simple overrides (e.g., the identity function on pointers) could be shared between them.
  • We decided we still like the notion of type aliases that are instantiated for particular target triples. Eventually, we may choose to have more pre-baked target triples in addition to x86_64 Linux.
  • We should make the parser match the language extension datatype as closely as possible. This means that we should leave out operations like fresh-vec. Ideally, the crucible-llvm-syntax and macaw-symbolic-syntax parsers would be extensible, so operations such as this would be possible to add in downstream packages.

@RyanGlScott Which parts of the above would you like to see done in this PR vs. deferred?

Copy link
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

This is fantastic. Thanks for digging through ambient-verifier and extracting this out!

(We ended up reviewing some of this patch synchronously, but I wrote some of these comments prior to the synchronous review. I've opted to keep the comments as-is, so if I ask questions below that were already answered during the review, that's why.)

symbolic-syntax/test/Test.hs Outdated Show resolved Hide resolved
symbolic-syntax/test/Test.hs Outdated Show resolved Hide resolved
symbolic-syntax/README.md Outdated Show resolved Hide resolved
symbolic-syntax/src/Data/Macaw/Symbolic/Syntax.hs Outdated Show resolved Hide resolved
symbolic-syntax/README.md Outdated Show resolved Hide resolved
symbolic-syntax/README.md Outdated Show resolved Hide resolved
symbolic-syntax/README.md Outdated Show resolved Hide resolved
symbolic-syntax/README.md Outdated Show resolved Hide resolved
symbolic-syntax/README.md Outdated Show resolved Hide resolved
symbolic-syntax/src/Data/Macaw/Symbolic/Syntax.hs Outdated Show resolved Hide resolved
@RyanGlScott
Copy link
Contributor

RyanGlScott commented Nov 2, 2023

Oops, we posted our comments at almost the exact same time. To respond to your specific questions:

  • The Pointer type now exists in crucible-llvm-syntax and macaw-symbolic-syntax. We should try to unify the surface syntax so that at least simple overrides (e.g., the identity function on pointers) could be shared between them.

See #345 (comment). I don't have a clear sense for how much work it would take to consolidate the two Pointer types, so if it looks daunting, I'm fine with deferring that for later. (There's also a question of whether we'd want to do this at all—I'm on the fence on that point.)

  • We decided we still like the notion of type aliases that are instantiated for particular target triples. Eventually, we may choose to have more pre-baked target triples in addition to x86_64 Linux.

Spot on. I suggested (in #345 (comment)) adding one for AArch32 Linux, since we have a more pressing need for that, but I'm fine with deferring other target triples.

  • We should make the parser match the language extension datatype as closely as possible. This means that we should leave out operations like fresh-vec. Ideally, the crucible-llvm-syntax and macaw-symbolic-syntax parsers would be extensible, so operations such as this would be possible to add in downstream packages.

Right, see #345 (comment). I think we should omit fresh-vec.

It's not part of the Macaw syntax, but rather an operation that's useful
when hand-writing CFGs. This should instead be supported by parser extensions
downstream.
These are no longer part of the base syntax, but can be added on.
@langston-barrett langston-barrett merged commit 7a70ce9 into master Nov 3, 2023
3 checks passed
@langston-barrett langston-barrett deleted the lb/syntax branch November 3, 2023 13:15
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.

Concrete syntax for macaw-symbolic CFGs, from ambient-verifier
2 participants