This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Store import errors for Decls
ClosedPublic

Authored by martong on May 24 2019, 2:31 AM.

Details

Summary

We add a new member (ImportedFromDecls) which is a mapping from the already-imported
declarations in the "from" context to the error status of the import of
that declaration. This map contains only the declarations that were not
correctly imported. The same declaration may or may not be included in
ImportedDecls. This map is updated continuously during imports and never
cleared (like ImportedDecls). In Import(Decl*) we use this mapping, so
if there was a previous failed import we return with the existing error.

We add/remove from the Lookuptable in consistency with ImportedFromDecls.
When we map a decl in the 'to' context to something in the 'from'
context then and only then we add it to the lookup table. When we
remove a mapping then and only then we remove it from the lookup table.

This patch is the first in a series of patches whose aim is to further
strengthen the error handling in ASTImporter.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.May 24 2019, 2:31 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
martong edited the summary of this revision. (Show Details)May 24 2019, 2:46 AM

Hi Gabor,
The idea looks fine to me, but I have some questions inline.

clang/lib/AST/ASTImporter.cpp
5109 ↗(On Diff #201163)

Is this FIXME obsolete now?

7823 ↗(On Diff #201163)

I see the enabled test case, but does it cover the logic in this block?

7851 ↗(On Diff #201163)

Is it possible to get this error more than once?

martong updated this revision to Diff 205811.Jun 20 2019, 7:42 AM
martong marked 3 inline comments as done.
  • Test error values are set for AST nodes
martong marked 3 inline comments as done.Jun 20 2019, 7:54 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
5109 ↗(On Diff #201163)

That is not obsolete. We should call HandleNameConflict as we do in other visit functions. But it is not trivial how to do that, there are open questions which I cannot address in this PR:

  • Should we pass D->getDeclName or rather the name of the imported ClassTemplateDecl or should we import the name here (again)?
  • Should we pass the template args to HandleNameConflict?
7823 ↗(On Diff #201163)

Right, that is not strictly related. I have added relevant unit test cases for the error handling. This branch is handled in ErrorHappensAfterCreatingTheNodeButBeforeLinkingThatToTheAST test.

7851 ↗(On Diff #201163)

Yes, that can happen in cyclic imports like: ClassTemplateDecl -> TemplatedDecl -> ClassTemplateDecl.

martong updated this revision to Diff 205814.Jun 20 2019, 7:55 AM
martong marked an inline comment as done.
  • Remove unrelated change
martong updated this revision to Diff 206017.Jun 21 2019, 9:15 AM
  • Remove formatv b/c it can't handle braces in code
martong updated this revision to Diff 206020.Jun 21 2019, 9:19 AM
  • Remove unused include and using
a_sidorin accepted this revision.Jun 23 2019, 10:40 PM
a_sidorin added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
4697 ↗(On Diff #206020)

#undef ERRONEOUSSTMT?

This revision is now accepted and ready to land.Jun 23 2019, 10:40 PM
balazske added inline comments.Jun 24 2019, 12:39 AM
clang/lib/AST/ASTImporter.cpp
7851 ↗(On Diff #201163)

I do not understand this completely, in this branch setImportDeclError is called so at next time we can not go into this branch again (for the same FromD). (We can get the error and not go into this branch more than once but get no error and go into the branch only once.)

clang/unittests/AST/ASTImporterTest.cpp
4697 ↗(On Diff #206020)

Maybe we should not use a macro for this, specially not define the macro inside the struct block. For the current tests it is sufficient to make a std::string that contains something like void f() { asm(""); }.

martong marked 4 inline comments as done.Jun 24 2019, 3:13 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
7851 ↗(On Diff #201163)

Yes, I missed that.
So, perhaps we can remove the condition if (!getImportDeclErrorIfAny(FromD)) ?

clang/unittests/AST/ASTImporterTest.cpp
4697 ↗(On Diff #206020)

My first attempt was to use std::string for the errouneous Stmt and then to use llvm::formatv to easily concatenate the test code examples.
However, it tunred out that formatv actively forbids the use of the braces in the source string.
So I decided to use a macro because that way we can build up the test code most easily.
What is the problem by defining the macro in a struct block? There are no blocks for the preprocessor, just directives. I'd like to indicate that the macro is used only by the test.

balazske added inline comments.Jun 24 2019, 4:17 AM
clang/lib/AST/ASTImporter.cpp
7851 ↗(On Diff #201163)

I think yes: If !ToDOrErr is true we have an error that was newly created in ImportImpl. But I am not totally sure, and the ImportImpl can be overridden, so it is better to not remove the condition. Or make an assert out of it: assert(!getImportDeclErrorIfAny(FromD) && "Import error already set for decl.").

clang/unittests/AST/ASTImporterTest.cpp
4697 ↗(On Diff #206020)

The problem was that it looks like that macro is an internal thing (to the struct) but in reality it is not. Probably std::string::append can be used if possible.

martong updated this revision to Diff 206218.Jun 24 2019, 8:01 AM
martong marked 2 inline comments as done.
  • Assert that we set the error only once
  • Remove the macro and use std::string.op+
martong marked 4 inline comments as done.Jun 24 2019, 8:03 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
7851 ↗(On Diff #201163)

Ok, I changed to have an assertion.

clang/unittests/AST/ASTImporterTest.cpp
4697 ↗(On Diff #206020)

Ok, I removed the macro and use rather std::string.operator+()

balazske accepted this revision.Jun 24 2019, 8:38 AM
martong marked 2 inline comments as done.Jun 25 2019, 1:03 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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2019, 1:04 AM