C++/C#: Make escape analysis unsound by default #2667
Conversation
When building SSA, we'll be assuming that stack variables do not escape, at least until we improve our alias analysis. I've added a new `IREscapeAnalysisConfiguration` class to allow the query to control this, and a new `UseSoundEscapeAnalysis.qll` module that can be imported to switch to the sound escape analysis. I've cloned the existing IR and SSA tests to have both sound and unsound versions. There were relatively few diffs in the IR dump tests, and the sanity tests still give the same results after one change described below. Assuming that stack variables do not escape exposed an existing bug where we do not emit an `Uninitialized` instruction for the temporary variables used by `return` statements and `throw` expressions, even if the initializer is a constructor call or array initializer. I've refactored the code for handling elements that initialize a variable to share a common base class. I added a test case for returning an object initialized by constructor call, and ensured that the IR diffs for the existing `throw` test cases are correct.
| @@ -0,0 +1 @@ | |||
| semmle/code/cpp/ir/implementation/aliased_ssa/PrintIR.ql | |||
There was a problem hiding this comment.
Why re-introduce the *aliased_ssa_ir.{qlref,expected} files? I thought you had a good reason to delete those in #919.
There was a problem hiding this comment.
Oops, I re-added those by mistake. They're useful for seeing diffs caused by changes, but they're still not worth keeping up to date with every PR. Fixed.
| exists(IREscapeAnalysisConfiguration config | | ||
| config.useSoundEscapeAnalysis() and | ||
| ( | ||
| automaticVariableAddressEscapes(var.(IRAutomaticVariable)) |
There was a problem hiding this comment.
Does the sound escape analysis get evaluated when config.useSoundEscapAnalysis doesn't hold?
There was a problem hiding this comment.
No. I checked the DIL, and the optimizer eliminates that half of the disjunction because it can prove that it never holds.
| @@ -43,93 +43,34 @@ abstract class TranslatedDeclarationEntry extends TranslatedElement, TTranslated | |||
| * Represents the IR translation of the declaration of a local variable, | |||
| * including its initialization, if any. | |||
| */ | |||
| abstract class TranslatedVariableDeclaration extends TranslatedElement, InitializationContext { | |||
| abstract class TranslatedLocalVariableDeclaration extends TranslatedVariableInitialization { | |||
There was a problem hiding this comment.
This looks like a rename without an associated change to what the values of the class are. Is that just a clarification?
| tag = InitializerVariableAddressTag() and | ||
| result = getInitialization().getFirstInstruction() and | ||
| kind instanceof GotoEdge | ||
| result = TranslatedVariableInitialization.super.getInstructionSuccessor(tag, kind) |
There was a problem hiding this comment.
I think I understand what's going on here (the successor relations within each piece are exactly the ones in the respective superclass, and the connection between them is injected into TranslatedVariableInitialization by getInitializationSuccessor) but it seems a bit spooky to me.
There was a problem hiding this comment.
I think a cleaner way of doing this would have been to make the actual (un)initialization part a separate element that is a child of the original element, i.e., composition instead of inheritance. That seemed like an even bigger refactoring in a change where I hadn't expected to do any refactoring to begin with. I can go back and improve this, but I'd prefer to do it in a follow-up post-January.
|
I've looked at the test expectation diffs, and they all seem to be expected. In most cases they are also better as well. |
|
I believe I've addressed all feedback and resolved all the merge conflicts. Ready for final sign-off. |
|
@dbartol Two tests are failing. |
|
Hmm. Not sure how I missed those. The diffs look like improvements thanks to the non-escaping of a local, so I've just accepted them. |
|
Tests are broken. Looks like a taint model change on master that affects the new tests in |
|
Fixed. |
When building SSA, we'll be assuming that stack variables do not escape, at least until we improve our alias analysis. I've added a new
IREscapeAnalysisConfigurationclass to allow the query to control this, and a newUseSoundEscapeAnalysis.qllmodule that can be imported to switch to the sound escape analysis. I've cloned the existing IR and SSA tests to have both sound and unsound versions. There were relatively few diffs in the IR dump tests, and the sanity tests still give the same results after one change described below.Assuming that stack variables do not escape exposed an existing bug where we do not emit an
Uninitializedinstruction for the temporary variables used byreturnstatements andthrowexpressions, even if the initializer is a constructor call or array initializer. I've refactored the code for handling elements that initialize a variable to share a common base class. I added a test case for returning an object initialized by constructor call, and ensured that the IR diffs for the existingthrowtest cases are correct.