Page MenuHomePhabricator

[ASTImporter] Mark erroneous nodes in shared st
ClosedPublic

Authored by martong on May 24 2019, 3:23 AM.

Details

Summary

This is the third step of the full error handling. We store the errors
for the Decls in the "to" context too. For that, however, we have to put
these errors in a shared state (among all the ASTImporter objects which
handle the same "to" context but different "from" contexts).

After a series of imports from different "from" TUs we have a "to" context
which may have erroneous nodes in it. (Remember, the AST is immutable so
there is no way to delete a node once we had created it and we realized
the error later.) All these erroneous nodes are marked in
ASTImporterSharedState::ImportErrors. Clients of the ASTImporter may
use this as an input. E.g. the static analyzer engine may not try to
analyze a function if that is marked as erroneous (it can be queried via
ASTImporterSharedState::getImportDeclErrorIfAny()).

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.May 24 2019, 3:23 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Hi Gabor!
I haven't find the import sequence examples we try to fix these ways in any of the three patches these change consists of. Could you please provide some (or point if I missed them)?

clang/include/clang/AST/ASTImporterSharedState.h
46 ↗(On Diff #201182)

= default?

clang/lib/AST/ASTImporter.cpp
7724 ↗(On Diff #201182)
if (auto* LookupTable = ..._)
  ...
    LookupTable->add();

looks a bit cleaner to me.

7830 ↗(On Diff #201182)

I'm not sure if it is safe to compare declarations from different TUs by pointers because they belong to different allocators.

7831 ↗(On Diff #201182)

getImportDeclErrorIfAny() is usually called for FromD, not for ToD. Is it intentional?

7867 ↗(On Diff #201182)

I think we can encapsulate these conditions into `SharedState::[add/remove]Decl[To/From]Lookup methods.

7924 ↗(On Diff #201182)

I don' think that an import failure from another TU means that the import from the current TU will fail.

Alexei,

Thanks for the review!
I have provided test cases for the 3 previous patches on which this one depends on. I will provide additional tests next week for this one, and of course will address the other comments.

martong updated this revision to Diff 206429.Tue, Jun 25, 6:35 AM
  • Add FIXMEs
  • Set error for FromD if it maps to an existing Decl which has an error set
  • Add test
martong updated this revision to Diff 206431.Tue, Jun 25, 6:44 AM
  • Set error for FromD if it maps to an existing Decl which has an error set
martong marked 6 inline comments as done.
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
7830 ↗(On Diff #201182)

In the SharedState we keep pointers only from the "to" context.

7831 ↗(On Diff #201182)

ASTImporter::getImportDeclErrorIfAny() is different from ASTImporterSharedState::getImportDeclErrorIfAny().

The former keeps track of the erroneous decls from the "from" context.

In the latter we keep track of those decls (and their error) which are in the "to" context.
The "to" context is common for all ASTImporter objects in the cross translation unit context.
They share the same ASTImporterLookupTable object too. Thus the name ASTImporterSharedState.

7924 ↗(On Diff #201182)

It should. At least if we map the newly imported decl to an existing but already messed up decl in the "to" context. Please refer to the added test case.

martong marked 7 inline comments as done.Tue, Jun 25, 7:15 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
7867 ↗(On Diff #201182)

Good catch, thanks!

martong updated this revision to Diff 206441.Tue, Jun 25, 7:16 AM
martong marked an inline comment as done.
  • Encapsulate by adding addDeclToLookup and removeDeclFromLookup to the shared state
a_sidorin accepted this revision.Sun, Jun 30, 4:24 AM

Thanks for the explanation!
It will be good if someone else takes a look at this patch.

clang/include/clang/AST/ASTImporterSharedState.h
40 ↗(On Diff #206441)

ErrorsInToContext?

This revision is now accepted and ready to land.Sun, Jun 30, 4:24 AM
balazske accepted this revision.Mon, Jul 1, 6:04 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jul 1, 8:37 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.

Jenkins had one unrelated failure at
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29812/
Test Result (1 failure / +1)
LLDB.Reproducer.TestGDBRemoteRepro.test

After that the next build is green:
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29813/