Fix for #1017: Compiler issues an invalid-bounds error message for an out-of-scope variable.#1031
Conversation
…ts are added to the CFG.
…unds. Also removing expressions that refer to out-of-scope variables from AvailableExpressions.
when variables go out of scope within a basic block.
… If one iterates through a loop and removes elements from it at the same time, the end of the iterator has to be re-evaluated in each iteration.
|
|
||
| EquivExprSets CrntEquivExprs(State.EquivExprs); | ||
| State.EquivExprs.clear(); | ||
| for (auto I = CrntEquivExprs.begin(); I != CrntEquivExprs.end(); ++I) { |
There was a problem hiding this comment.
Since CrntEquivExprs is not mutated inside the loop, a more efficient way to write this loop is as follows:
for (auto I = CrntEquivExprs.begin(), E = CrntEquivExprs.end(); I != E; ++I)
This avoids repeated evaluation of CrntEquivExprs.end() each time through the loop.
See https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
| // CFGElement. When a variable goes out of scope, ObservedBounds and | ||
| // EquivExprs in the CheckingState have to be updated. | ||
| CFGLifetimeEnds LE = Elem.castAs<CFGLifetimeEnds>(); | ||
| VarDecl *V = const_cast<VarDecl *>(LE.getVarDecl()); |
There was a problem hiding this comment.
It shouldn't be null but I have added check to ensure this.
There was a problem hiding this comment.
If LE.getVarDecl() is null const_cast<VarDecl *>(LE.getVarDecl()) will crash. It may be better to use dyn_cast_or_null instead of const_cast.
There was a problem hiding this comment.
If
LE.getVarDecl()is nullconst_cast<VarDecl *>(LE.getVarDecl())will crash. It may be better to usedyn_cast_or_nullinstead ofconst_cast.
In an offline discussion Sulekha pointed to this link (https://en.cppreference.com/w/cpp/language/const_cast) which indicates that "a null pointer value would be converted to a null pointer value of new_type". So this code should work fine and there is no need to explicitly check if LE.getVarDecl() is null before invoking const_cast.
dtarditi
left a comment
dtarditi
left a comment
There was a problem hiding this comment.
@sulekhark, what is the effect on compile-time of this change?
In the future, we might want to optimize how deletion is done for equivalent expressions, given that the deletion operation is being done every time a variable lifetime ends. I'm not requesting that we optimize it right now. I'm curious about the effect on compile-time of this change.
|
@dtarditi Compile time increases by 2.35% . Here are the details: |
dtarditi
left a comment
dtarditi
left a comment
There was a problem hiding this comment.
Thanks for measuring the effect on compile-time and opening an issue to track looking at that later. A 2% compile-time increase is something that we can live with for now, so I approve the change.
4 participants
Intersecting the set of in-scope variables across the predecessors of a basic block excludes most variables that are not in-scope for the basic block. However, an exception is when a variable goes out of scope within the basic block. When this happens, the bug in #1017 manifests.
Fix:
Enabling the
AddLifetimeflag during CFG construction produces theCFGLifetimeEndsCFGElementfor every variable that goes out of scope. When we encounter thisCFGElement, we remove the variable that goes out of scope from theObservedBoundsmap if it is a checked pointer, and also delete all available expressions that use this out-of-scope variable.