don't consume fast math flag entry #171
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The record entries within a `FUNC_CODE_INST_PHI` are such that there is 1 type entry, then 2 entries per argument, and optionally a fast math entry. The code for consuming the pairs of argument entries was incorrectly checking whether there remained entries, and so was attempting to read the fast math entry as part of an argument pair, when that entry is present. Instead, it should stop whenever there are either 0 or 1 entries remaining, as the last entry is definitely a fast math one.
atomb
approved these changes
Oct 12, 2021
RyanGlScott
added a commit
to GaloisInc/crucible
that referenced
this pull request
Dec 5, 2021
This bumps the `llvm-pretty` submodule to bring in the following PRs: * GaloisInc/llvm-pretty#86 (support LLVM 11 `IsDefault` fields in metadata) * GaloisInc/llvm-pretty#87 (Relax the Symbol requirement to `ConstBlockAddr`) * GaloisInc/llvm-pretty#88 (add LLVM12 poison value) This also bumps the `llvm-pretty-bc-parser` submodule to bring in the following PRs: * GaloisInc/llvm-pretty-bc-parser#171 (don't consume fast math flag entry) * GaloisInc/llvm-pretty-bc-parser#169 (make `getBitString` assumptions explicit) * GaloisInc/llvm-pretty-bc-parser#168 (support for LLVM 11 `IsDefault` fields in metadata) * GaloisInc/llvm-pretty-bc-parser#170 (remove `Aligned` from `SubWord`) * GaloisInc/llvm-pretty-bc-parser#172 (fix BLOCKADDRESS parsing) * GaloisInc/llvm-pretty-bc-parser#173 (parse LLVM12 poison value) * GaloisInc/llvm-pretty-bc-parser#178 (use Seq over list in PartialModule) Of these, the only PR that requires code changes in Crucible is GaloisInc/llvm-pretty#88 (add LLVM12 poison value), as this adds a new `ValPoison` constructor to `Value'`. Properly handling LLVM poison is a large task that I do not wish to tackle here (see #366 for discussion of how one might do this). For now, this PR does the bare minimum: * In `Lang.Crucible.LLVM.Translation.BlockInfo.useVal`, treat `ValPoison` as not referencing any identifiers. * The other two potential sites where `ValPoison` could be matched on are in `Lang.Crucible.LLVM.Translation.Constant.transConstant'` and `Lang.Crucible.LLVM.Translation.Expr.transValue`. Since we are not handling `ValPoison` for now, both of these functions will simply error if they encounter `ValPoison`. * I have added a section to `crucible-llvm`'s `limitations.md` document which describes the above limitations of poison handling.
RyanGlScott
added a commit
to GaloisInc/crucible
that referenced
this pull request
Dec 6, 2021
This bumps the `llvm-pretty` submodule to bring in the following PRs: * GaloisInc/llvm-pretty#86 (support LLVM 11 `IsDefault` fields in metadata) * GaloisInc/llvm-pretty#87 (Relax the Symbol requirement to `ConstBlockAddr`) * GaloisInc/llvm-pretty#88 (add LLVM12 poison value) * GaloisInc/llvm-pretty#90 (Support structs with bitfields in `Text.LLVM.DebugUtils`) This also bumps the `llvm-pretty-bc-parser` submodule to bring in the following PRs: * GaloisInc/llvm-pretty-bc-parser#171 (don't consume fast math flag entry) * GaloisInc/llvm-pretty-bc-parser#169 (make `getBitString` assumptions explicit) * GaloisInc/llvm-pretty-bc-parser#168 (support for LLVM 11 `IsDefault` fields in metadata) * GaloisInc/llvm-pretty-bc-parser#170 (remove `Aligned` from `SubWord`) * GaloisInc/llvm-pretty-bc-parser#172 (fix BLOCKADDRESS parsing) * GaloisInc/llvm-pretty-bc-parser#173 (parse LLVM12 poison value) * GaloisInc/llvm-pretty-bc-parser#178 (use Seq over list in PartialModule) Of these, the only PR that requires code changes in Crucible is GaloisInc/llvm-pretty#88 (add LLVM12 poison value), as this adds a new `ValPoison` constructor to `Value'`. Properly handling LLVM poison is a large task that I do not wish to tackle here (see #366 for discussion of how one might do this). For now, this PR does the bare minimum: * In `Lang.Crucible.LLVM.Translation.BlockInfo.useVal`, treat `ValPoison` as not referencing any identifiers. * The other two potential sites where `ValPoison` could be matched on are in `Lang.Crucible.LLVM.Translation.Constant.transConstant'` and `Lang.Crucible.LLVM.Translation.Expr.transValue`. Since we are not handling `ValPoison` for now, both of these functions will simply error if they encounter `ValPoison`. * I have added a section to `crucible-llvm`'s `limitations.md` document which describes the above limitations of poison handling.
RyanGlScott
added a commit
to GaloisInc/crucible
that referenced
this pull request
Dec 6, 2021
This bumps the `llvm-pretty` submodule to bring in the following PRs: * GaloisInc/llvm-pretty#86 (support LLVM 11 `IsDefault` fields in metadata) * GaloisInc/llvm-pretty#87 (Relax the Symbol requirement to `ConstBlockAddr`) * GaloisInc/llvm-pretty#88 (add LLVM12 poison value) * GaloisInc/llvm-pretty#90 (Support structs with bitfields in `Text.LLVM.DebugUtils`) This also bumps the `llvm-pretty-bc-parser` submodule to bring in the following PRs: * GaloisInc/llvm-pretty-bc-parser#171 (don't consume fast math flag entry) * GaloisInc/llvm-pretty-bc-parser#169 (make `getBitString` assumptions explicit) * GaloisInc/llvm-pretty-bc-parser#168 (support for LLVM 11 `IsDefault` fields in metadata) * GaloisInc/llvm-pretty-bc-parser#170 (remove `Aligned` from `SubWord`) * GaloisInc/llvm-pretty-bc-parser#172 (fix BLOCKADDRESS parsing) * GaloisInc/llvm-pretty-bc-parser#173 (parse LLVM12 poison value) * GaloisInc/llvm-pretty-bc-parser#178 (use Seq over list in PartialModule) Of these, the only PR that requires code changes in Crucible is GaloisInc/llvm-pretty#88 (add LLVM12 poison value), as this adds a new `ValPoison` constructor to `Value'`. Properly handling LLVM poison is a large task that I do not wish to tackle here (see #366 for discussion of how one might do this). For now, this PR does the bare minimum: * In `Lang.Crucible.LLVM.Translation.BlockInfo.useVal`, treat `ValPoison` as not referencing any identifiers. * The other two potential sites where `ValPoison` could be matched on are in `Lang.Crucible.LLVM.Translation.Constant.transConstant'` and `Lang.Crucible.LLVM.Translation.Expr.transValue`. Since we are not handling `ValPoison` for now, both of these functions will simply error if they encounter `ValPoison`. * I have added a section to `crucible-llvm`'s `limitations.md` document which describes the above limitations of poison handling.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The record entries within a
FUNC_CODE_INST_PHIare such that there is 1 typeentry, then 2 entries per argument, and optionally a fast math entry.
The code for consuming the pairs of argument entries was incorrectly checking
whether there remained entries, and so was attempting to read the fast math
entry as part of an argument pair, when that entry is present.
Instead, it should stop whenever there are either 0 or 1 entries remaining, as
the last entry is definitely a fast math one.