[Lldb-commits] [lldb] [lldb][ClangExpressionParser] Implement ExternalSemaSource::ReadUndefinedButUsed (PR #104817)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

<details>
<summary>Changes</summary>

While parsing an expression, Clang tries to diagnose usage of decls (with possibly non-external linkage) for which it hasn't been provided with a definition. This is the case, e.g., for functions with parameters that live in an anonymous namespace (those will have `UniqueExternal` linkage). Before diagnosing such situations, Clang calls `ExternalSemaSource::ReadUndefinedButUsed`. The intended use of this API is to extend the set of "used but not defined" decls with additional ones that the external source knows about. However, in LLDB's case, we never provide `FunctionDecl`s with a definition, and instead rely on the expression parser to resolve those symbols by linkage name. Thus, to avoid the Clang parser from erroring out in these situations, this patch implements `ReadUndefinedButUsed` which just removes the "undefined" non-external `FunctionDecl`s that Clang found.

We also had to add an `ExternalSemaSource` to the `clang::Sema` instance LLDB creates. We previously didn't have any source on `Sema`. Because we add the `ExternalASTSourceWrapper` here, that means we'd also technically be adding the `ClangExpressionDeclMap` as an `ExternalASTSource` to `Sema`, which is fine because `Sema` will only be calling into the `ExternalSemaSource` APIs (though nothing currently strictly enforces this, which is a bit worrying).

Fixes https://github.com/llvm/llvm-project/issues/104712

---
Full diff: https://github.com/llvm/llvm-project/pull/104817.diff


3 Files Affected:

- (modified) lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h (+19) 
- (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+3) 
- (added) lldb/test/Shell/Expr/TestAnonNamespaceParamFunc.cpp (+19) 


``````````diff
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
index 79ed74b79f04fd..9710f7dd4e983c 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
@@ -14,6 +14,7 @@
 #include "clang/Sema/MultiplexExternalSemaSource.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Sema/SemaConsumer.h"
+#include "llvm/Support/Casting.h"
 #include <optional>
 
 namespace clang {
@@ -135,6 +136,24 @@ class ExternalASTSourceWrapper : public clang::ExternalSemaSource {
     return m_Source->layoutRecordType(Record, Size, Alignment, FieldOffsets,
                                       BaseOffsets, VirtualBaseOffsets);
   }
+
+  /// This gets called when Sema is reconciling undefined but used functions.
+  /// For LLDB's use-case, we never provide Clang with function definitions,
+  /// instead we rely on linkage names and symbol resolution to call the
+  /// correct funcitons during JITting. So this implementation clears
+  /// any "undefined" functions Clang found while parsing.
+  ///
+  /// \param[in,out] Undefined A set of used decls for which Clang has not
+  ///                          been provided a definition with.
+  ///
+  void ReadUndefinedButUsed(
+      llvm::MapVector<clang::NamedDecl *, clang::SourceLocation> &Undefined)
+      override {
+    Undefined.remove_if([](auto const &decl_loc_pair) {
+      const clang::NamedDecl *ND = decl_loc_pair.first;
+      return llvm::isa_and_present<clang::FunctionDecl>(ND);
+    });
+  }
 };
 
 /// Wraps an ASTConsumer into an SemaConsumer. Doesn't take ownership of the
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index c441153d1efb05..b23e851f547ce9 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1223,6 +1223,8 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
 
     clang::ExternalASTSource *ast_source = decl_map->CreateProxy();
 
+    auto *ast_source_wrapper = new ExternalASTSourceWrapper(ast_source);
+
     if (ast_context.getExternalSource()) {
       auto module_wrapper =
           new ExternalASTSourceWrapper(ast_context.getExternalSource());
@@ -1236,6 +1238,7 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
     } else {
       ast_context.setExternalSource(ast_source);
     }
+    m_compiler->getSema().addExternalSource(ast_source_wrapper);
     decl_map->InstallASTContext(*m_ast_context);
   }
 
diff --git a/lldb/test/Shell/Expr/TestAnonNamespaceParamFunc.cpp b/lldb/test/Shell/Expr/TestAnonNamespaceParamFunc.cpp
new file mode 100644
index 00000000000000..750c81c0db6b32
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestAnonNamespaceParamFunc.cpp
@@ -0,0 +1,19 @@
+// TODO: Test that ...
+
+// RUN: %build %s -o %t
+// RUN: %lldb %t -o run -o "expression func(a)" -o exit | FileCheck %s
+
+// CHECK: expression func(a)
+// CHECK: (int) $0 = 15
+
+namespace {
+struct InAnon {};
+} // namespace
+
+int func(InAnon a) { return 15; }
+
+int main() {
+  InAnon a;
+  __builtin_debugtrap();
+  return func(a);
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/104817