[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

Mon Jan 7 04:14:01 PST 2019

sammccall added a comment.

In D54309#1320628 <https://reviews.llvm.org/D54309#1320628>, @JonasToth wrote:

> this patch "broke" `ExprMutAnalyzer`, at least it creates an assertion failure that seems harmless. Your thought on it would be great!

As mentioned on the bug, this assertion already existed and this patch tries to preserve it in as many cases as possible.
You may be uncovering some unrelated bug (the kind the assertion was meant to catch). Does the assertion go away by reverting this patch (and keeping the old version of the assert)?

I've tried to check myself, but I can't use either of your reproducers. I'll try the one from the bug next (https://bugs.llvm.org/show_bug.cgi?id=39949)

> The assertion in: `ASTMatchFinder.cpp +680` is triggered in the Analysis to figure out if something is mutated, because `Parents` is empty and the traversal scope is the TU itself, but the node is a `CXXConstructoDecl`. This does not happen for alle cases though, please take a look at this reproducer: https://godbolt.org/z/0yOq2I
> 
> Only for the Lambda-Case problems occur. If I `#if 0` the assertion out the issue goes away, and the real world code that triggered this behaviour is not bothered, too.

Oops, took me a while to realize this is a test for a check that hasn't landed :-)
The fact that this only triggers for a lambda case does suggest to me it's unrelated to this patch - those are exactly the sort of cases where apparently AST traversal has been incomplete in the past, yielding a partial parent map. My understanding is this tends to be harmless where it actually fires, but points to an AST inconsistency that is likely to cause problems in other cases. Thus there's pushback against removing the assertion (which I initially attempted to do).

> I am really puzzled why this issue occurs, but as you implemented the restrictions it would like to hear your opinion on the issue. If you want to reproduce yourself I am of course happy to help, but 
>  https://github.com/JonasToth/clang/blob/fix_crash/unittests/Analysis/ExprMutationAnalyzerTest.cpp#L1134   is probably the easiest way to do so (just add this to trunk and run `check-clang-unit`).

Is this still current? The code fails to parse (on 1137 `a&&` should be `T&&`). When I fix that the `Results11 = match(withEnclosingCompound(...))` yields an empty vector. (The test does subsequently crash, but not in an interesting way).

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54309/new/

https://reviews.llvm.org/D54309