[all-commits] [llvm/llvm-project] ab8b22: [lldb] Don't create duplicate declarations when co...

[Raphael Isemann via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: ab8b22d1c2d97b1e50c73b8640c3acb192652059
      https://github.com/llvm/llvm-project/commit/ab8b22d1c2d97b1e50c73b8640c3acb192652059
  Author: Raphael Isemann <teemperor at gmail.com>
  Date:   2020-01-29 (Wed, 29 Jan 2020)

  Changed paths:
    M lldb/source/Symbol/ClangASTImporter.cpp
    M lldb/unittests/Symbol/TestClangASTImporter.cpp

  Log Message:
  -----------
  [lldb] Don't create duplicate declarations when completing a forward declaration with a definition from another source

Summary:
I noticed this strange line in `ASTImporterDelegate::ImportDefinitionTo` which doesn't make a lot of sense:
```
to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
```

It forcibly sets the imported TagDecl to be defined if the source TagDecl was defined. This doesn't make any
sense as in this code we already forced the ASTImporter to import the definition so this should always be
a no-op.

Turns out this is hiding two bugs:
1. The way we handle forward declarations in the debug info that might be completed later is that we
  import them and then mark them as having external lexical storage. This makes Clang ask for the definition
  later when it needs it (at which point we hopefully have the definition around and can complete it). However,
  this is currently not completing the forward decls with external storage but instead creates a duplicated decl
  in the target AST which is then defined. The forward decl is kept incomplete after the import and we just
  forcibly make it a definition of the record without any content with our workaround. The TestSharedLib* tests
  is only passing because of this.
2. Minimal import of lambdas is broken and never imports the definition it seems. That appears to be a bug
  in the ASTImporter which gives the definition of lambda's some special treatment. TestLambdas.py is actually
  broken but is passing because of this workaround.

This patch fixes the first bug by forcing the ASTImporter to import to the target forward declaration. We can't
delete the workaround as the second bug is still around but that will be a follow up review for the ASTImporter.
However it will get rid of all the duplicated RecordDecls that are in our expression AST that are strangely defined
but don't have any of the fields they are supposed to have.

Reviewers: shafik, labath

Reviewed By: shafik

Subscribers: aprantl, abidh, JDevlieghere, lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D73345