[expression] Explicitly build the lookup table of any TagDecl's context

[martong]

Summary

Currently the lookup table for the context of TagDecls is not built. This can
cause various lookup failures. These lookup failures cause that the ast
importer creates multiple but equivalent declarations. The solution is to
explicitly force the creation of the lookup tables right after the given
declarations are imported. This way subsequent imports will find the existing
declaration.

[martong]

There is a lookup failure within the
ASTImporter during the expression evaluation. The clang::ASTImporter cannot
lookup previously created declarations thus it creates duplicated and redundant
declarations. These duplicated declarations then later can cause different
assertions.

More specifically, the problem occurs in the presence of a LinkageSpecDecl.
Here are the steps to reproduce:

• Compile a simple test binary (a.out)
• Launch LLDB with a.out
• set a breakpoint
• run
• expr -l objc++ -- @import Darwin; 3
• expr *fopen("/dev/zero", "w")

When we evaluate the fopen expression then we import the __sFILE
struct several times to the same ASTContext.
(To see these we have to attach to the original lldb process with
another debugger and add breakpoints to
ASTNodeImporter::VisitRecordDecl.)
After the first import we have this in the AST:

TranslationUnitDecl
`-LinkageSpecDecl 0x7ff9eab97618 <<module-includes>:76:1, line:78:1> line:76:8 C
  |-CXXRecordDecl 0x7ff9eab97668
</Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/_stdio.h:126:9,
col:16> col:16 <undeserialized declarations> struct __sFILE definition
  | `-DefinitionData pass_in_registers aggregate standard_layout
trivially_copyable pod trivial literal
  |   |-DefaultConstructor exists trivial needs_implicit
  |   |-CopyConstructor simple trivial has_const_param needs_implicit
implicit_has_const_param
  |   |-MoveConstructor exists simple trivial needs_implicit
  |   |-CopyAssignment trivial has_const_param needs_implicit
implicit_has_const_param
  |   |-MoveAssignment exists simple trivial needs_implicit
  |   `-Destructor simple irrelevant trivial needs_implicit
  `-TypedefDecl 0x7ff9eab977f8 <col:1, line:157:3> col:3 FILE 'struct
__sFILE':'__sFILE'
    `-ElaboratedType 0x7ff9eab977a0 'struct __sFILE' sugar
      `-RecordType 0x7ff9eab97710 '__sFILE'
        `-CXXRecord 0x7ff9eab97668 '__sFILE'

During the second import the lookup fails thus we end up with a
duplicated CXXRecordDecl of __sFILE in the AST:

TranslationUnitDecl
|-LinkageSpecDecl 0x7ff9eab97618 <<module-includes>:76:1, line:78:1> line:76:8 C
| |-CXXRecordDecl 0x7ff9eab97668
</Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/_stdio.h:126:9,
col:16> col:16 <undeserialized declarations> struct __sFILE definition
| | `-DefinitionData pass_in_registers aggregate standard_layout
trivially_copyable pod trivial literal
| |   |-DefaultConstructor exists trivial needs_implicit
| |   |-CopyConstructor simple trivial has_const_param needs_implicit
implicit_has_const_param
| |   |-MoveConstructor exists simple trivial needs_implicit
| |   |-CopyAssignment trivial has_const_param needs_implicit
implicit_has_const_param
| |   |-MoveAssignment exists simple trivial needs_implicit
| |   `-Destructor simple irrelevant trivial needs_implicit
| `-TypedefDecl 0x7ff9eab977f8 <col:1, line:157:3> col:3 FILE 'struct
__sFILE':'__sFILE'
|   `-ElaboratedType 0x7ff9eab977a0 'struct __sFILE' sugar
|     `-RecordType 0x7ff9eab97710 '__sFILE'
|       `-CXXRecord 0x7ff9eab97668 '__sFILE'
`-LinkageSpecDecl 0x7ff9eab97888 <<module-includes>:76:1, line:78:1> line:76:8 C
  `-CXXRecordDecl 0x7ff9eab978d8
</Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk/usr/include/_stdio.h:126:9,
col:16> col:16 <undeserialized declarations> struct __sFILE definition
    `-DefinitionData pass_in_registers aggregate standard_layout
trivially_copyable pod trivial literal
      |-DefaultConstructor exists trivial needs_implicit
      |-CopyConstructor simple trivial has_const_param needs_implicit
implicit_has_const_param
      |-MoveConstructor exists simple trivial needs_implicit
      |-CopyAssignment trivial has_const_param needs_implicit
implicit_has_const_param
      |-MoveAssignment exists simple trivial needs_implicit
      `-Destructor simple irrelevant trivial needs_implicit

The reason behind this is the following:
ASTImporter uses DeclContext::localUncachedLookup on a redecl context,
because we must not perform a lookup in a transparent context (there
is an assertion for that in DeclContext).
The redecl context of the __sFILE CXXRecordDecl is the
TranslationUnitDecl, while the normal DC is the LinkageSpecDecl (which
is a transparent context).
localUncachedLookup searches via the lookup table (lookupPtr) of the
given DeclContext, but only if that DC does not have external lexical
storage!
However, in the LLDB case it does have, so we end up in the Slow case
of the localUncachedLookup:

// Slow case: grovel through the declarations in our chain looking for
// matches.
// FIXME: If we have lazy external declarations, this will not find them!
// FIXME: Should we CollectAllContexts and walk them all here?
for (Decl *D = FirstDecl; D; D = D->getNextDeclInContext()) {
  if (auto *ND = dyn_cast<NamedDecl>(D))
    if (ND->getDeclName() == Name)
      Results.push_back(ND);
}

Since the DC is the redecl DC (TranslationUnitDecl) and not the normal
DC (LinkageSpecDecl), we will not find the CXXRecordDecl with the name
__sFILE.

[clayborg]

Might be good for Jim Ingham to look at this as well?

[martong]

Ping

[clayborg]

Any way to dump the AST in a test to verify we don't create multiple?

[martong]

In D59826#1451536, @clayborg wrote:

Any way to dump the AST in a test to verify we don't create multiple?

I think I might be able to use the log object to dump the AST and then from python it would be possible to check for duplicates. Is that a good direction?

[clayborg]

In D59826#1452771, @martong wrote:

In D59826#1451536, @clayborg wrote:

Any way to dump the AST in a test to verify we don't create multiple?

I think I might be able to use the log object to dump the AST and then from python it would be possible to check for duplicates. Is that a good direction?

yes that would be great!

[labath]

In D59826#1452771, @martong wrote:

In D59826#1451536, @clayborg wrote:

Any way to dump the AST in a test to verify we don't create multiple?

I think I might be able to use the log object to dump the AST and then from python it would be possible to check for duplicates. Is that a good direction?

note there's also a new "target modules dump ast" command, which might be of help here. You can see it in action in the NativePDB tests (lit/SymbolFile/NativePDB).

[martong]

During writing the tests, I realized that this is only a partial solution. So I abandon this patch in favor for https://reviews.llvm.org/D61333 , please take a look at that.

source/Symbol/ClangASTImporter.cpp

if (log)
                   static_cast<void *>(&to->getASTContext()));
   }
 
   if (TagDecl *from_tag_decl = dyn_cast<TagDecl>(from)) {
     TagDecl *to_tag_decl = dyn_cast<TagDecl>(to);
 
     to_tag_decl->setHasExternalLexicalStorage();
     to_tag_decl->getPrimaryContext()->setMustBuildLookupTable();
+    if (!to_tag_decl->getPrimaryContext()->isTransparentContext())
+      if (to_tag_decl->isCompleteDefinition())
+        to_tag_decl->buildLookup();
+
+    DeclContext *redecl_primary_ctx =
+        to_tag_decl->getRedeclContext()->getPrimaryContext();
+    redecl_primary_ctx->setMustBuildLookupTable();
+    if (to_tag_decl->isCompleteDefinition())
+      redecl_primary_ctx->buildLookup();
 
     if (log)
       log->Printf(
           "    [ClangASTImporter] To is a TagDecl - attributes %s%s [%s->%s]",
           (to_tag_decl->hasExternalLexicalStorage() ? " Lexical" : ""),
           (to_tag_decl->hasExternalVisibleStorage() ? " Visible" : ""),
           (from_tag_decl->isCompleteDefinition() ? "complete" : "incomplete"),
           (to_tag_decl->isCompleteDefinition() ? "complete" : "incomplete"));