Fix order of IR call side effects #6601
Conversation
…Call`. This is the first step to fixing the order of side effects on call instructions. The goal is to move all side effects (argument side effects, allocation side effects, and conservative call side effects) to be treated as elements in a single sequence of side effects, which will then be handled in a single place similar to how we already handle argument side effects.
This is step two of fixing the ordering of call side effects. This commit refactors the existing `TranslatedSideEffect` class into an abstract `TranslatedSideEffect` class, which contains functionality common to all kinds of side effect, and a concrete `TranslatedArgumentSideEffect` class, which is the implementation of argument side effects. A future commit will add additional concrete classes for conservative call side effects and allocation side effects. This change has zero diffs to the generated IR.
…rguments This commit is yet another step to fixing the order of IR side effect instructions. Instead of having a special `StructorCallSideEffects` class for the call itself, I've introduced a `TranslatedStructorCallQualifierSideEffect` class that shares a bunch of common code with `TranslatedArgumentExprSideEffect`, but handles the case where there's no `Expr` for the qualifier of the constructor call. Because this class uses the same ordering as regular argument side effects, these side effects now appear in the correct order, reads before writes. The test expectations have changed to reflect the new, correct order.
…other side effects during IR generation This commit moves the IR generation for the `CallSideEffect` and `InitializeDynamicAllocation` side effect instruction into their own subclasses of `TranslatedSideEffect`. Previously, they were embeddded in `TranslatedCall` and `TranslatedAllocationSideEffects`. There are no diffs in the generated IR. This just makes the implementation of all side effect generation be consistent.
MathiasVP
left a comment
MathiasVP
left a comment
There was a problem hiding this comment.
Only took a quick look at the changes so far. I'll continue tomorrow!
cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/SideEffects.qll
Outdated
Show resolved
Hide resolved
|
The changes LGTM! I'll wait for:
Does this PR also fix https://github.com/github/codeql-c-team/issues/596? |
It does, but I couldn't remember if we were OK with linking to private issues from public repos. I guess the cat's out of the bag now, though, so I'll make it official. |
rdmarsh2
left a comment
rdmarsh2
left a comment
There was a problem hiding this comment.
One minor point for readability, but I'm happy with merging as-is
| * call in a `new` or `new[]` expression. | ||
| */ | ||
| class TranslatedCallSideEffects extends TranslatedSideEffects, TTranslatedCallSideEffects { | ||
| Call expr; |
There was a problem hiding this comment.
Should this be CallOrAllocation expr;?
|
@dbartol I see two requests for you here:
|
|
We should make sure this PR doesn't fall between the cracks. |
|
@MathiasVP @rdmarsh2 DCA experiment is done: https://github.com/github/codeql-dca-main/tree/data/dbartol/pr-6601-9183a4d__Differences__code-scanning/reports A couple of non-trivial analysis time increases, in ChakraCore and putty, but no analysis diffs. |
|
@MathiasVP @rdmarsh2 Are the two slowdowns significant enough to worry about? |
Probably not. If you don't see anything obvious in the tuple logs I think we should just merge it as-is. FWIW, the |
|
Those runs have bigger relative slowdowns in database build time than analysis time, which makes me suspect it's a machine issue, although it's weird that it's that consistent. |
4 participants
This PR fixes the order in which side effect instructions are emitted for calls such that all read-only side effects always appear before any write side effect. Previously, for historical implementation reasons, we always emitted the
CallSideEffectinstruction immediately after theCallinstruction, even if there were specific argument read side effects that might read from memory that overlaps with the memory written byCallSideEffect.The approach is to reuse the implementation we already were already using for argument indirection side effects, where a single placeholder
TranslatedSideEffectsobject has a sequence ofTranslatedSideEffectobjects as children. All four kinds of side effect (argument indirection, constructor call qualifier, conservativeCallSideEffectinstruction, and dynamic memory allocation) now just insert themselves into that sequence ofTranslatedSideEffects, sorted in an order that ensures that all read side effects happen before any write side effects.I suggest reviewing this PR commit-by-commit. The first couple of commits do a bit of refactoring with zero IR diffs to prepare for the main changes. The next couple commits move the existing side effect implementations into the new common implementation. Only one of the commits actually changes the order of the side effects in the IR.
Fixes https://github.com/github/codeql-c-team/issues/596