RecursiveASTVisitor: Traverse the child of a BoundsValueExpr

[kkjeer]

Fixes #974
Fixes #977

This PR modifies RecursiveASTVisitor to traverse the child (E->getTemporaryBinding) of a BoundsValueExpr by default. This allows the CollectVarSetHelper in SemaBounds.cpp to determine the correct set of variables involved in expressions that contain a BoundsValueExpr. For example, in the expression BoundsValue(TempBinding(x)) + y, CollectVarSetHelper will return the set { x, y } rather than { y }.

Two other classes in SemaBounds.cpp that use RecursiveASTVisitor now override this behavior so that they do not traverse the temporary binding of a BoundsValueExpr:

• NonModifyingExprSema: expressions within bounds values should not be considered modifying.
• VariableCountHelper: variables within bounds values should not contribute to the variable occurrence count within an expression. The ReplaceVariable method ReplaceVariable(E, V, OriginalValue) returns the original expression E if VariableCountHelper returns 0 as the variable occurrence count of V in E. If V occurs within a BoundsValueExpr within E, then ReplaceVariable should return E without traversing it. Therefore, for an expression such as BoundsValue(TempBinding(x)), VariableCountHelper should return 0 as the occurrence count of x.

Testing:

• Added test cases to clang/test/CheckedC/static-checking/free-variables.c that involve expressions containing bounds values. These test cases cover the example in Wrong error from #903 when passing _Assume_bounds_cast directly to function with itype #974.

[kkjeer]

Some more details about the changes in this PR and the relevant parts of the bounds checker:

Problem Summary

The part of the bounds checker that detects free variables does the
following for expressions E1 and E2 that are involved in bounds expressions
D and S. (For example, E1 and E2 may be the base expressions of D and S.
If the declared bounds D are bounds(p, p + 1) and the source bounds S are
bounds(q, q + 1), then E1 and E2 could be p and q, respectively.)

1. Get the set of variables involved in E1. Let this set be Vars1.
2. Get the set of variables involved in E2. Let this set be Vars2.
3. For each variable V in Vars1:
4. Check whether V is equal to an IntegerLiteral. If so, V is not a
   free variable.
5. Check whether V is equal to some variable U in Vars2. If so, V is not
   a free variable.
6. Check whether there is an indirect relationship between V and some variable
   U in Vars2. If so, V is not a free variable.
7. Repeat steps 3-6, checking each variable in Vars2 against the set Vars1.

Steps 1 and 2 use a helper class called CollectVarSetHelper that returns the
set of all (unique) DeclRefExprs within an expression E. For example, if E is
(p * 2) + p + q + 3, then CollectVarSetHelper will return the set { p, q }
for E.

CollectVarSetHelper extends RecursiveASTVisitor to traverse an expression E and
overrides the VisitDeclRefExpr method to add a DeclRefExpr to the set of
variables in E.

A BoundsValueExpr has one child: a CHKCBindTemporaryExpr, obtained by
calling E->getTemporaryBinding() for a BoundsValueExpr E. By default,
RecursiveASTVisitor does not traverse E->getTemporaryBinding(). Suppose we
have the following base expressions E1 and E2 for declared and inferred bounds:

• E1 = a
• E2 = BoundsValue(TempBinding(a))

CollectVarSetHelper will return { a } for E1. However, for E2,
RecursiveASTVisitor will not traverse the TempBinding(a) child of the
BoundsValue. The VisitDeclRefExpr method will never be called, and a
will never be added to the set of variables, so CollectVarSetHelper will
return {} for E2. Then, when it is time to detect free variables between
the sets { a } and {}, a will be treated as a free variable.

Solution: Modify Default Behavior of RecursiveASTVisitor

The fix for the above problem is to modify RecursiveASTVisitor so that, for
a BoundsValueExpr E, E->getTemporaryBinding() is traversed by default.
This is consistent with the behavior of RecursiveASTVisitor for other kinds of
expressions, which is to fully traverse all children of the expression.

There are three classes in SemaBounds.cpp which use RecursiveASTVisitor which
need to override this behavior and not traverse the child of a BoundsValueExpr.

NonModifyingExprSema

The NonModifyingExprSema class uses RecursiveASTVisitor to traverse an expression
E to check whether E contains any modifying expressions (e.g. assignments,
function calls, etc.). Any expressions that are wrapped in a BoundsValueExpr
should not be considered modifying, so NonModifyingExprSema should not traverse
the child of a BoundsValueExpr. For example, consider:

array_ptr<int> getArr(void) : count(2);

void f(void) {
  array_ptr<int> arr : count(2) = getArr();
}

The inferred bounds of arr are bounds(BoundsValue(TempBinding(getArr())), BoundsValue(TempBinding(getArr())) + 2).
The getArr() function call within the BoundsValue expression should not be treated as a
modifying expression - if it is, this program will not compile. (Modifying expressions in bounds result in compile-time errors.)

VariableCountHelper

The UpdateAfterAssignment method in the bounds checker updates all observed
variable bounds after an assignment to a variable V with an original value
OV before the assignment. If V appears in the observed bounds B of a
variable W, then the observed bounds of W are set to B1. B1 is the
result of replacing all occurrences of V in B with OV.

The VariableCountHelper class uses RecursiveASTVisitor to traverse an expression
E to determine the number of times a DeclRefExpr V appears in E.
The ReplaceVariableHelper class uses TreeTransform to traverse an expression
E to replace all occurrences of a DeclRefExpr V with an expression OV.
If VariableCountHelper returns 0 for E and V, ReplaceVariableHelper returns
the original expression E with no traversals and no modifications.

For expressions which contain BoundsValueExprs, any variables within a
BoundsValueExpr should not be replaced and should not count toward any
variable occurrence counts for the expression. For example, consider:

void f(array_ptr<int> arr : count(1)) {
  arr = _Dynamic_bounds_cast<array_ptr<int>>(arr, count(2));
}

The inferred bounds of the RHS of the assignment are
bounds((array_ptr<int>)BoundsValue(TempBinding(arr)), (array_ptr<int>)BoundsValue(TempBinding(arr)) + 2),
where the (array_ptr<int>) casts are bit casts.
The original value of arr is (array_ptr<int>)arr, where the (array_ptr<int>)
cast is a no-op cast.
The uses of arr in the RHS bounds should not be replaced with the original value
(array_ptr<int>)arr. The RHS bounds should remain unmodified.

[sulekhark]

LGTM.

[dtarditi]

LGTM, thanks!

[dtarditi]

LGTM, thanks!