[Safe Buffers][BoundsSafety] Fix a bug in the interop analysis that can cause infinite loops

[ziqingluo-90]

The interop analysis substitutes formal parameters to arguments in AST visiting. Conceptually, the substitution should be done exactly once. But the previous implementation never checked if the substitution has happened. In cases where the argument contains DREs referring to the Decl of the corresponding formal parameter, the analysis enters an infinite loop. A typical example is the following.

void f(int * __counted_by(n) p, size_t n) {
  f(p, n);
}

This commit fixes the issue.

(rdar://145705060)

[ziqingluo-90]

CC @dtarditi @jkorous-apple

[hnrklssn]

It's not clear to me what the iteration order is, could you explain what this class does? Does it visit everything as intended in something like this? If nothing else, I think it would be good to have test cases with shared variables, and the same variable appearing multiple times in the same expression.

static void foo(int * __counted_by(n + n * m) p, size_t n, int * __counted_by(m * m + l) q, int * __counted_by(l) r, size_t m, size_t o) {
  foo(p, n, q, r, m, o);
}

[ziqingluo-90]

I've added this test.

Let me try to explain it with more context.
We need to compare an expression after applying a mapping from formal parameters to actual arguments against another expression. In your example, one of the comparisons is

(n + n * m)[n->n, m->m]  vs. n + n * m

where the left-hand side represents the expected length of the formal parameter p of the callee, which needs to be applied with actual arguments (I.e., parameter n maps to argument n, etc.); the right-hand side is the actual length of the argument p inferred by our analysis through p's counted-by type.
(Note that symbols like p, n', m` play different roles---parameter and argument according to the context in this recursive situation.)

Our implementation uses a visitor to traverse the two comparing expressions e1 vs. e2 and apply the substitution when it visits a DRE of a formal parameter in e1. Naturally, it must make sure each reference to a formal parameter in e1 gets substituted exactly once, otherwise it will enter an infinite loop in the example above. This is what the bug is.

The fix lets the visitor take an extra argument representing whether a sub-expression being visited is the result of substitution. If so, no substitution shall happen during the visit of the sub-expression.
For example, suppose we are visiting an expression v + v with a mapping {v -> a + a, a -> b}.
The visitor first visits the LHS v and replaces it with a + a. It then visits a + a with the knowledge that no substitution should happen for a + a. Without the fix, the visitor will erroneously replace a with b. Since the information is passed by an argument, it will not affect the visits of other AST branches. So when the visitor goes back to visit the RHS v, it knows correctly that v needs to be substituted to a + a again.

[hnrklssn]

Thanks, that makes sense to me!

[patrykstefanski]

Wouldn't it be simpler to keep this flag as a member variable and then use llvm::SaveAndRestore to set the flag to true for recursive calls? In this case, you wouldn't have to propagate it in each call.

[ziqingluo-90]

yeah, using a global flag results in much less code change.
Adding a parameter to those visit methods is my personal preference because I feel it naturally indicates that the flag only affects sub-expressions during the visit. While using a global flag requires the programmer to not forget to manage the flag whenever they attempt to make a substitution.

Consider this is a somewhat urgent fix, let's merge it as is for now? If this visitor grows more complicated later, we re-evaluate which approach is better.