[Lldb-commits] [PATCH] D59826: [expression] Explicitly build the lookup table of any TagDecl's context

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

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`.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59826/new/

https://reviews.llvm.org/D59826