-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C++: Flow out of invalid functions #12005
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
C++: Flow out of invalid functions #12005
Conversation
…ing a return statement.
return instanceof ReturnInstruction or | ||
return instanceof UnreachedInstruction | ||
| | ||
block.getInstruction(index) = return and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index + 1
has been replaced here by index
. Was this incorrect before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good question. It wasn't actually incorrect. The only thing we require is that it's the last use of the parameter in the body of the function. ssa0/SsaInternals.qll
did index + 1
, and SsaInternals.qll
did index
. So I just synced them up to be index
for consistency. This will hopefully make it easier to merge those two files into one parameterized module eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index + 1
might actually be problematic since we can generate IR where the UnreachedInstruction
is the only thing in the block. So there wouldn't necessarily be any instruction at block.getInstruction(index + 1)
.
This is different from ReturnInstructions
since we know there is always a ExitFunctionInstruction
(or whatever it's called) after the ReturnInstructions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM provided DCA is happy and we now have the two missing results back in our internal tests.
The internal test still isn't recovered, but I think there's an additional step necessary for this. I'll write up my investigation in our internal issue for this. |
9573395
into
github:mathiasvp/replace-ast-with-ir-use-usedataflow
We were not getting flow out of functions that were missing
return
statements since they might not have aReturnInstruction
.After this PR, we (ab)use the generated
UnreachedInstruction
to assign an index in the final basic block so that SSA will ensure that we have flow out of the (invalid) function.