Bounds context: observed bounds [2/n] #827
Conversation
clang/lib/Sema/SemaBounds.cpp
Outdated
| @@ -683,12 +685,14 @@ namespace { | |||
| SemaRef(SemaRef), | |||
| BoundsContextRef(Context) {} | |||
|
|
|||
| bool VisitDeclaratorDecl(DeclaratorDecl *D) { | |||
| bool VisitVarDecl(VarDecl *D) { | |||
clang/lib/Sema/SemaBounds.cpp
Outdated
| @@ -683,12 +685,14 @@ namespace { | |||
| SemaRef(SemaRef), | |||
| BoundsContextRef(Context) {} | |||
|
|
|||
| bool VisitDeclaratorDecl(DeclaratorDecl *D) { | |||
| bool VisitVarDecl(VarDecl *D) { | |||
There was a problem hiding this comment.
Could you add a comment about what the bool return value of this function means?
| BoundsExpr *Bounds = D->getBoundsExpr(); | ||
| if (Bounds) | ||
| BoundsContextRef[D] = Bounds; | ||
| BoundsContextRef[D] = SemaRef.ExpandBoundsToRange(D, Bounds); |
There was a problem hiding this comment.
Could you add a comment here that the declared bounds are normalized to range bounds here?
clang/lib/Sema/SemaBounds.cpp
Outdated
| @@ -820,19 +824,19 @@ namespace { | |||
| DumpEqualExpr(OS, State.G); | |||
| } | |||
|
|
|||
| void DumpBoundsContext(raw_ostream &OS, BoundsContextTy UC) { | |||
| if (UC.empty()) | |||
| void DumpBoundsContext(raw_ostream &OS, BoundsContextTy Context) { | |||
There was a problem hiding this comment.
Can you change the Context parameter to be a reference?
void DumpBoundsContext(raw_ostream &OS, BoundsContextTy &Context)
clang/lib/Sema/SemaBounds.cpp
Outdated
| DeclaratorDecl *Variable = *I; | ||
| if (!UC[Variable]) | ||
| const VarDecl *Variable = *I; | ||
| if (!Context[Variable]) |
There was a problem hiding this comment.
I think this would insert the Variable in Context if it does not exist.
If you only want to test existence of key in a map please use this:
if (Context.count(Variable))
If you want to test existence and use the value, then use this:
auto it = Context.find(Variable);
if (it != Context.end()) {
// use *it
}
clang/lib/Sema/SemaBounds.cpp
Outdated
| BoundsContextTy Context2) { | ||
| BoundsContextTy IntersectedContext; | ||
| for (auto Pair : Context1) { | ||
| if (!Pair.second || !Context2[Pair.first]) |
There was a problem hiding this comment.
Same comment here about checking for existence of key in map.
| for (auto Pair : UC1) { | ||
| if (!Pair.second || !UC2[Pair.first]) | ||
| // intersection of the contexts Context1 and Context2. | ||
| BoundsContextTy IntersectBoundsContexts(BoundsContextTy Context1, |
There was a problem hiding this comment.
Can this be a templated function which can be used to intersect sets holding items of any type? See https://github.com/microsoft/checkedc-clang/blob/master/clang/lib/Sema/BoundsAnalysis.cpp#L686.
Going forward we should move these intersect, union, etc functions to a common file so that we can re-use them.
There was a problem hiding this comment.
I updated IntersectBoundsContext to reset each in-scope variable to its normalized declared bounds. Now that it's more specialized to rely on variable declarations and bounds expressions, I don't think it lends itself as well to templating.
|
The names UEQ, G, OV, etc sound very cryptic. Is it possible to rename them to more descriptive names in a separate PR? |
Created #828 to track this. |
This PR replaces the UC bounds context member of CheckingState with ObservedBounds, and uses the observed bounds as the rvalue bounds for the value of a variable, rather than as the lvalue target bounds for a variable.
Context:
This is part of the ongoing work to improve the soundness of bounds checking involving assignments to variables. This work is also related to the bounds widening analysis (e.g. #821). After this PR is merged, and the ObservedBounds context is updated to reflect the widened bounds of nt_array_ptrs, the bounds checking can use the widened bounds as the rvalue bounds of the value of an nt_array_ptr variable.
Notable changes:
Tests: