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

Give call/callbr function types instead of pointer types #207

Merged
merged 1 commit into from
Apr 3, 2023
Merged

Conversation

RyanGlScott
Copy link
Contributor

See Note [Typing function applications] for the rationale.

This fixes #189. This is also an important step towards being able to support opaque pointers, as described in #177.

RyanGlScott added a commit to GaloisInc/crucible that referenced this pull request Mar 20, 2023
There are a handful of places in `crucible-llvm` where we assume that the
`Call` and `CallBr` instructions have pointer types. As noted in
GaloisInc/llvm-pretty-bc-parser#189, however, this is subtly incorrect. This
patch:

* Bumps the `llvm-pretty-bc-parser` submodule to bring in the changes from
  GaloisInc/llvm-pretty-bc-parser#207, which corrects this mistake, and
* Changes the parts of `crucible-llvm` that match on pointer type to no longer
  do so.
RyanGlScott added a commit to GaloisInc/crucible that referenced this pull request Mar 20, 2023
There are a handful of places in `crucible-llvm` where we assume that the
`Call` and `CallBr` instructions have pointer types. As noted in
GaloisInc/llvm-pretty-bc-parser#189, however, this is subtly incorrect. This
patch:

* Bumps the `llvm-pretty-bc-parser` submodule to bring in the changes from
  GaloisInc/llvm-pretty-bc-parser#207, which corrects this mistake.
* Bumps the `llvm-pretty` submodule to be in tandem with
  `llvm-pretty-bc-parser`.
* Changes the parts of `crucible-llvm` that match on pointer type to no longer
  do so.
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.

I'm aligned with the changes, but is there a way to add a test to at least validate the parse/print round-trip is valid (and agrees with llvm-dis)?

@RyanGlScott
Copy link
Contributor Author

I'm glad you prompted me to look, because it turns out that llvm-pretty is making the same pointer-type/function-type confusion here in its pretty-printer:

ppCallSym :: LLVM => Type -> Value -> Doc
ppCallSym (PtrTo (FunTy res args va))   val        = ppType res <+> ppArgList va (map ppType args) <+> ppValue val
ppCallSym ty              val                      = ppType ty  <+> ppValue val

We shouldn't be using PtrTo here. I will need to submit an llvm-pretty PR to address this.

@RyanGlScott RyanGlScott marked this pull request as draft March 20, 2023 21:53
src/Data/LLVM/BitCode/IR/Function.hs Outdated Show resolved Hide resolved
@RyanGlScott
Copy link
Contributor Author

I have submitted GaloisInc/llvm-pretty#104 with the necessary changes to support pretty-printing call instructions properly in an opaque pointer setting. I will update the llvm-pretty submodule after that lands.

Because bumping the llvm-pretty submodule will require bringing in the changes from GaloisInc/llvm-pretty#103, I will most likely need to base this PR on top of #202 to make everything compile.

See `Note [Typing function applications]` for the rationale. I have added a
`T189.ll` test case to ensure that we actually practice what we preach in that
Note.

This fixes #189. This is also an important step towards being able to support
opaque pointers, as described in #177.
@RyanGlScott RyanGlScott marked this pull request as ready for review April 3, 2023 12:29
@RyanGlScott
Copy link
Contributor Author

I've added a T189.ll test case to kick the tires and make sure that the parser and pretty-printer treats function types the way that they should. As a result, this is now ready for review.

(Note that I haven't yet added a test case that uses opaque pointers, as I will do that in a separate patch.)

@RyanGlScott RyanGlScott merged commit d541adf into master Apr 3, 2023
RyanGlScott added a commit to GaloisInc/crucible that referenced this pull request Apr 3, 2023
There are a handful of places in `crucible-llvm` where we assume that the
`Call` and `CallBr` instructions have pointer types. As noted in
GaloisInc/llvm-pretty-bc-parser#189, however, this is subtly incorrect. This
patch:

* Bumps the `llvm-pretty-bc-parser` submodule to bring in the changes from
  GaloisInc/llvm-pretty-bc-parser#207, which corrects this mistake.
* Bumps the `llvm-pretty` submodule to be in tandem with
  `llvm-pretty-bc-parser`. Note that because this brings in the changes from
  GaloisInc/llvm-pretty#104, I opted to explicitly disallow LLVM's opaque
  pointers in `crucible-llvm` for now, but this is something that will likely
  change in subsequent patches.
* Changes the parts of `crucible-llvm` that match on pointer types to no longer
  do so.
@RyanGlScott RyanGlScott deleted the T189 branch May 30, 2023 12:22
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.

call/callbr's function type should not be a pointer type
3 participants