This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] import SubStmt of CaseStmt
ClosedPublic

Authored by r.stahl on Oct 16 2017, 12:49 AM.

Details

Summary

This fixes importing of CaseStmts, because the importer was not importing its SubStmt.

A test was added and the test procedure was adjusted to also dump the imported Decl, because otherwise the bug would not be detected.

Diff Detail

Event Timeline

r.stahl created this revision.Oct 16 2017, 12:49 AM
xazax.hun accepted this revision.Oct 16 2017, 4:22 AM

LGTM with a nit.

unittests/AST/ASTImporterTest.cpp
100

I would elaborate a bit more on the purpose of the code below.

This revision is now accepted and ready to land.Oct 16 2017, 4:22 AM

Thanks for the fast response. See inline comment.

unittests/AST/ASTImporterTest.cpp
100

I will need help for that, since I do not have a better rationale myself. My guess would be that print has different assumptions and error checking than dump, so executing both increases coverage.

xazax.hun added inline comments.Oct 16 2017, 8:16 AM
unittests/AST/ASTImporterTest.cpp
100

So the reason you need this to traverse the AST and crash on the null pointers (when the patch is not applied right?)
I think you could comment something like traverse the AST to catch certain bugs like poorly (not) imported subtrees.
If we do not want to actually print the whole tree (because it might be noisy) you can also try using a no-op recursive AST visitor.

r.stahl updated this revision to Diff 119162.Oct 16 2017, 8:45 AM
r.stahl marked 3 inline comments as done.

addressed review comment

If all is good, I will need someone to commit this for me please.

unittests/AST/ASTImporterTest.cpp
100

Yes, of course this referred to the case when the patch is not applied.

There is no noise since the dump is done on a buffer that will be discarded.

This revision was automatically updated to reflect the committed changes.
a.sidorin added inline comments.Jan 30 2018, 12:08 PM
cfe/trunk/unittests/AST/ASTImporterTest.cpp
100 ↗(On Diff #119444)

I just saw this change and I cannot find the reason, why do we need to print the imported declaration after we have printed the entire translation unit? Is there some special case?

r.stahl added inline comments.Mar 27 2018, 12:30 AM
cfe/trunk/unittests/AST/ASTImporterTest.cpp
100 ↗(On Diff #119444)

Sorry for the late reply.

The particular bug here would only be detected when dumping, but not when printing. The output of printing was just not listing the missing AST nodes, but dumping crashed and therefore failed the test as expected.

I'm not familiar with the inner workings of print and dump, so I cannot explain why that is.

martong added inline comments.Mar 27 2018, 1:24 AM
cfe/trunk/unittests/AST/ASTImporterTest.cpp
100 ↗(On Diff #119444)

I have a rough guess that this might be because the code buffer that is used in vfs::InMemoryFileSystem has a reference to the actual code string. That actual code string is a string literal in most of the cases which should be in the data segment, but there is no guarantee for that. E.g. some optimizing compilers might put it onto the stack.
We experienced a similar issue during building a test Fixture (https://reviews.llvm.org/D43967).

What I suggest is to build the ASTTests target with asan and ubsan enabled (set LLVM_USE_SANITIZER for cmake) and trigger the dump which caused the bug.