This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Propagate error from ImportDeclContext
ClosedPublic

Authored by martong on Jun 20 2019, 7:39 AM.

Details

Summary

During analysis of one project we failed to import one
CXXDestructorDecl. But since we did not propagate the error in
importDeclContext we had a CXXRecordDecl without a destructor. Then the
analyzer engine had a CallEvent where the nonexistent dtor was requested
(crash).

Solution is to propagate the errors we have during importing a
DeclContext.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Jun 20 2019, 7:39 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

I don't have the insight to LGTM the whole change - just a nit about implementation detail.

clang/lib/AST/ASTImporter.cpp
1724 ↗(On Diff #205810)

You might consider using just a lambda with llvm::make_scope_exit.

https://llvm.org/doxygen/namespacellvm.html#a4896534f3c6278be56967444daf38e3f

For example:

To->startDefinition();
auto DefinitionCompleter = llvm::make_scope_exit( [To](){To->completeDefinition();} );
martong updated this revision to Diff 206018.Jun 21 2019, 9:17 AM
  • Remove formatv b/c it can't handle braces in code
balazske added inline comments.Jun 24 2019, 12:59 AM
clang/unittests/AST/ASTImporterTest.cpp
4743 ↗(On Diff #206018)

Maybe add a similar test for namespace and check that the error is not propagated?

martong updated this revision to Diff 206223.Jun 24 2019, 8:31 AM
  • Remove the macro and use std::string.op+
martong marked 4 inline comments as done.Jun 24 2019, 9:59 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
1724 ↗(On Diff #205810)

Thanks! I have changed to make_scope_exit.

clang/unittests/AST/ASTImporterTest.cpp
4743 ↗(On Diff #206018)

Ok, I have added that test.

martong updated this revision to Diff 206250.Jun 24 2019, 9:59 AM
martong marked 2 inline comments as done.
  • Use make_scope_exit
  • Add test ErrorIsNotPropagatedFromMemberToNamespace
a_sidorin accepted this revision.Jun 30 2019, 9:53 AM

Hi Gabor,
The patch looks good. Thanks!

This revision is now accepted and ready to land.Jun 30 2019, 9:53 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 5:44 AM

@shafik I've been looking for any lldb regression in our Mac machine, could not find any. Now I am looking at http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ . I don't expect regression because here we changed logic about the error handling only, so I'd expect to have regression only if we had testcases in lldb for erroneous cases, but apparently there are no such tests.