C++/C#: Fix a couple new consistency failures, and improve consistency messages #3719
Conversation
This file contains hidden or 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
added 3 commits
June 15, 2020 10:46
Some of our IR consistency failure query predicates already produced results in the schema as an `@kind problem` query, including `$@` replacements for the enclosing `IRFunction` to make it easier to figure out which function to dump when debugging. This change moves the rest of the query predicates in `IRConsistency.qll` to do the same. In addition, it wraps each call to `getEnclosingIRFunction()` to return an `OptionalIRFunction`, which can be either a real `IRFunction` or a placeholder in case `getEnclosingIRFunction()` returned no results. This exposes a couple new consistency failures in `syntax-zoo`, which will be fixed in a subsequent commit. This change also deals with consistency failures when the enclosing `IRFunction` has more than one `Function` or `Location`. For multiple `Function`s, we concatenate the function names. For multiple `Location`s, we pick the first one in lexicographical order. This changes the number of results produced in the existing tests, but does't change the actual number of problems.
`TTranslatedAllocationSideEffects` wasn't limiting itself to functions that actually have IR, so it was getting used even in template definitions.
We were creating a `TranslatedFunction` even for functions that were not from source code, but then telling the IR package that those functions didn't have IR. This resulted in having prologue/epilogue instructions (e.g. `EnterFunction`, `ExitFunction`) with no enclosing `IRFunction`.
jbj
approved these changes
Jun 16, 2020
| // To avoid an overwhelming number of results when the extractor merges functions with the | ||
| // same name, just pick a single location. | ||
| result = | ||
| rank[1](Language::Location loc | loc = irFunc.getLocation() | loc order by loc.toString()) |
There was a problem hiding this comment.
rank[1] can also be spelled min
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.
This PR introduces a new IR consistency check (for
Instructions with zero or multiplegetEnclosingIRFunction()results), fixes the existing instances of those, and improves the output of the consistency queries to handle missing and duplicate results when creating the actual failure result tuples.Fixed failures
C++ -
TTranslatedAllocationSideEffectswasn't limiting itself to functions that actually have IR, so it was getting used even in template definitions.C# - We were creating a
TranslatedFunctioneven for functions that were not from source code, but then telling the IR package that those functions didn't have IR. This resulted in having prologue/epilogue instructions (e.g.EnterFunction,ExitFunction) with no enclosingIRFunction.Improved output
Some of our IR consistency failure query predicates already produced results in the schema as an
@kind problemquery, including$@replacements for the enclosingIRFunctionto make it easier to figure out which function to dump when debugging. This change moves the rest of the query predicates inIRConsistency.qllto do the same. In addition, it wraps each call togetEnclosingIRFunction()to return anOptionalIRFunction, which can be either a realIRFunctionor a placeholder in casegetEnclosingIRFunction()returned no results. This exposes a couple new consistency failures insyntax-zoo, which will be fixed in a subsequent commit.This change also deals with consistency failures when the enclosing
IRFunctionhas more than oneFunctionorLocation. For multipleFunctions, we concatenate the function names. For multipleLocations, we pick the first one in lexicographical order. This changes the number of results produced in the existing tests, but does't change the actual number of problems.