This is an archive of the discontinued LLVM Phabricator instance.

[AST] Traverse the class type loc inside the member pointer type loc.
ClosedPublic

Authored by hokein on Nov 29 2019, 4:14 AM.

Diff Detail

Event Timeline

hokein created this revision.Nov 29 2019, 4:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2019, 4:14 AM
hokein retitled this revision from [AST] Traverse the class type loc inside the member type loc. to [AST] Traverse the class type loc inside the member pointer type loc..Nov 29 2019, 4:33 AM
ilya-biryukov added inline comments.Nov 29 2019, 6:40 AM
clang/include/clang/AST/RecursiveASTVisitor.h
1164–1165

Remove this FIXME

1168–1170

Can this actually happen in practice?
Did we try doing only TRY_TO(TraverseTypeLoc(TSI->getTypeLoc())) and seeing whether this fails?

The code in MemberPointerTypeLoc suggests this can happen, but would be interesting to find examples when it happens.

hokein updated this revision to Diff 231649.Dec 2 2019, 1:58 AM
hokein marked 3 inline comments as done.

address review comments.

ilya-biryukov added inline comments.Dec 2 2019, 5:05 AM
clang/include/clang/AST/RecursiveASTVisitor.h
1168–1170

Our tests probably don't catch the case where it's null, right?
Could we add one? (Temporarily removing the check for null should be enough to trigger a crash here)

hokein marked an inline comment as done.Dec 2 2019, 7:49 AM
hokein added inline comments.
clang/include/clang/AST/RecursiveASTVisitor.h
1168–1170

I tried to add one for this, but failed to figure out a case that would trigger this code path (the existing tests are all passed if we remove this if guard check.)

any thoughts?

hokein updated this revision to Diff 232323.Dec 5 2019, 5:25 AM

use the assertion.

hokein marked an inline comment as done.Dec 5 2019, 5:28 AM
hokein added inline comments.
clang/include/clang/AST/RecursiveASTVisitor.h
1168–1170

per offline discussion, we don't find out such an example (all tests are passed), it seems that we may never encounter this cases. Added an assertion that would allow us to catch it.

Build result: pass - 60507 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

This revision is now accepted and ready to land.Dec 5 2019, 5:49 AM
This revision was automatically updated to reflect the committed changes.
saugustine added inline comments.
clang/include/clang/AST/RecursiveASTVisitor.h
1168–1170

This can happen when traversing a TypeNodeLoc, and the assertion will fail. I'm reverting this change for now.

hokein marked an inline comment as done.Dec 6 2019, 4:30 AM
hokein added inline comments.
clang/include/clang/AST/RecursiveASTVisitor.h
1168–1170

This can happen when traversing a TypeNodeLoc, and the assertion will fail. I'm reverting this change for now.

Thanks for reverting it, could you share more details? would be nice if you could point us an example that triggers the assertion, I believe it was found when you did the llvm buildcop.

rnk added a subscriber: rnk.Dec 6 2019, 11:05 AM

We also hit this, and I have a repro here, but it depends on the Chromium Blink GC plugin:
https://bugs.chromium.org/p/chromium/issues/detail?id=1031274#c4
I think you just need an RAV client that walks over some particular AST node to trigger the assert. I assume there is nothing special about the Blink GC plugin here.

Thanks again Sterling for the revert, more bots are green this morning as a result.