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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to