[all-commits] [llvm/llvm-project] 502773: [lldb][NFC] Remove ImportInProgress lock in ClangA...

[Raphael Isemann via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: 502773d743417a7896e184b4000bcf20fddb7ad9
      https://github.com/llvm/llvm-project/commit/502773d743417a7896e184b4000bcf20fddb7ad9
  Author: Raphael Isemann <teemperor at gmail.com>
  Date:   2020-06-30 (Tue, 30 Jun 2020)

  Changed paths:
    M lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
    M lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.h
    M lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

  Log Message:
  -----------
  [lldb][NFC] Remove ImportInProgress lock in ClangASTSource

Summary:

The ClangASTSource has a lock that globally disables all lookups into the
external AST source when we explicitly "guarded" copy a type. It's not used for
anything else, so importing declarations or importing types that are
dependencies of a declaration actually won't activate that lock. The lookups it
is supposed to prevent also don't actually happen in our test suite. The check
in `ClangExpressionDeclMap::FindExternalVisibleDecls` is never executed and the
check in the `ClangASTSource::FindExternalVisibleDeclsByName` is only ever
reached by the `Import-std-module` tests (which explicitly do a lookup into the
expression context on purpose).

This lock was added in 6abfabff6158076eccdf6fcac5a12894039de2c9 as a replacement
for a list of types we already looked up which appeared to be an optimisation
strategy. I assume back then this lock had a purpose but these days the
ASTImporter and LLDB seem to be smart enough to avoid whatever lookups this
tried to prevent.

I would say we remove it from LLDB. The main reason is that it blocks D81561
(which explicitly does a specific lookup to resolve placeholder types produced
by `-flimit-debug-info`) but it's semantics are also very confusing. The naming
implies it's a flag to indicate when we import something at the moment which is
practically never true as described above. Also the fact that it makes our
ExternalASTSource alternate between doing lookups into the debug info and
pretending it doesn't know any external decls could really break our lookup in
some weird way if Clang decides to cache a fake empty lookup result that was
generated while the lock was active.

Reviewers: labath, shafik, JDevlieghere, aprantl

Reviewed By: labath, JDevlieghere, aprantl

Subscribers: aprantl, abidh

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