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
Fix for #1026: Enabling LifetimeEnds CFGElement crashes the compiler during CFG construction #1028
Conversation
// Get E's ancestor that has the same LocalScope as one of B's ancestors. | ||
LocalScope::const_iterator PofE = E.shared_parent(B); | ||
int Dist = B.distance(PofE); | ||
if (Dist <= 0) |
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.
Can the distance be less than 0?
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.
While Dist
cannot be less than 0 if the distance
function is called as intended, it is possible to provide arguments to it such that the distance between two LocalScope iterators is less than zero. For example, this can happen if the control comes here for a "forward" jumping goto
statement (which should not ideally happen). Moreover, this kind of check is there in two other places too and they have used a <=
check (see lines 1770 - 1777).
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.
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.
@sulekhark and I discussed this change. Overall, looks good. I suggested using full variables names instead of abbreviations like PofB
. Also, this is not a parent, but an ancestor of the scope.
I thought the comment was somewhat confusing because it doesn't explain is actually happening. We find a common shared scope and end the lifetime of any variables defined in the scopes nested within the shared scope that enclose the goto statement. If you goto a scope A from a scope B that lexically scope A, the shared scope is scope B, so no variable definitions end. I understand the comment is taken from another place in the file; the original comment is confusing.
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.
Looks good, thanks.
The crash happens in some scenarios where a
goto
statement has to be backpatched withCFGLifetimeEnds
elements. During CFG construction, the statements enclosed within a compound statement are iterated through in reverse (i.e., last statement is processed first). In the case where the target of thegoto
statement occurs earlier than thegoto
, thegoto
statement is processed before its target labeled statement is processed. Now, the variables that go out of scope when thegoto
is executed depend on the position of the target of thegoto
statement. Therefore, theLifetimeEnds
CFGElements
for these variables, in the case mentioned above, have to be backpatched into the basic block containing thegoto
only after the target of thegoto
has been processed.The bug is in determining the set of variables that go out of scope, while performing the backpatching. Consider the following block structure:
The variables that go out of scope when the
goto
is executed are:D
andC
. The above block structure represents the general case that is not handled during backpatching. The cases that are handled are when the scope of thegoto
target is an ancestor of the scope that contains thegoto
statement in the tree of scopes within the function.