Use the widened bounds to update bounds in context #808
Conversation
clang/lib/Sema/SemaBounds.cpp
Outdated
| // widened bounds for the current block. | ||
| for (const auto item : KilledBounds) { | ||
| for (const VarDecl *V : item.second) | ||
| WidenedBounds.erase(V); |
There was a problem hiding this comment.
Does this kill bounds widened in the current block even for statements that occur before the bounds are killed? E.g.
void f(nt_array_ptr p : count(len), int len) {
if (*p) { // widen bounds of p
p[1]; // bounds of p should still be widened here
len = 0; // kill the widened bounds of p
}
}
It seems like the widened bounds of p will be killed for the entire block, before the statement len = 0; kills them.
There was a problem hiding this comment.
Yes, my original intent was to remove from WidenedBounds only the bounds killed by the current statement. But the problem is that BlockState.UC would also need to be updated to set the bounds for the VarDecl to the declared bounds.
For this GetDeclaredBounds would need to be called outside the loop and the context would need to be stored in a declared context.
Otherwise, this case does not work:
nt_array_ptr p : count(0);
int i;
if (*p) {
i = 0;
p++;
if (*(p + 1)) {}
}
Here, the BlockState.UC is updated with p:1 inside the block if (*p). Then the bounds are killed at p++. But the BlockState.UC still has p:1. So in this case we also need to reset the context with the declared bounds for p which are p:0.
|
In the latest change set, I have fixed the handling of bounds killed by a statement. For bounds killed by a statement, we reset the bounds to the declared bounds. So we first gather the declared bounds and then store them in a declared bounds context. Then when we find that the bounds for a variable are killed by a statement we reset those bounds from the declared context. We still do not handle the following case: The reason the above case is not handled is that the upper bound of p is (p + len). But the checker does not currently understand that the dereference should only be at the upper bound. The bounds widening algo correctly handles this by not widening the bounds after the dereference. |
| @@ -622,11 +622,15 @@ void BoundsAnalysis::ComputeOutSets(ElevatedCFGBlock *EB, | |||
| } | |||
|
|
|||
| StmtDeclSetTy BoundsAnalysis::GetKillSet(const CFGBlock *B) { | |||
| if (!BlockMap.count(B)) | |||
There was a problem hiding this comment.
Is it more efficient to use find and the check the result than to use count? Otherwise are you are looking up the key twice in the map. For a hash table, this can be expensive.
| if (!BoundsCtx.count(V)) | ||
| continue; | ||
|
|
||
| BoundsExpr *CurrBoundsExpr = BoundsCtx[V]; |
There was a problem hiding this comment.
I don't understand why you are skipping the variable if an entry already exists in BoundsCtx. Shouldn't you always add the widened bounds in BoundsMapTy?
| // If the declared bounds are a RangeBoundsExpr, add the widened bounds | ||
| // to the upper bounds of the range. | ||
| } else if (auto *RBE = dyn_cast<RangeBoundsExpr>(CurrBoundsExpr)) { | ||
| Expr *Upper = RBE->getUpperExpr(); |
There was a problem hiding this comment.
You should not be modifying the existing range bounds expression. Please create new one.
| WB.second); | ||
|
|
||
| auto *WidenedBoundsExpr = | ||
| new (Context) CountBoundsExpr(BoundsExpr::Kind::ElementCount, |
There was a problem hiding this comment.
The underlying code prefers to work on range bounds expressions. It just takes count expressions and converts them range bounds expressions anyway. I'd suggest for efficiency that you create a range bounds expression.
|
Closing this in favor of #821 |
Use the widened bounds calculated by the analysis in BoundsAnalysis.cpp to
widen the bounds of nt_array_ptr.