Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb] DWARFDIE: Follow DW_AT_specification when computing CompilerCo… #77157

Merged
merged 1 commit into from Jan 9, 2024

Conversation

adrian-prantl
Copy link
Collaborator

…ntext

Following the specification chain seems to be clearly the expected behavior of GetDeclContext(). Otherwise C++ methods have an empty CompilerContext instead of being nested in their struct/class.

Theprimary motivation for this functionality is the Swift plugin. In order to test the change I added a proof-of-concept implementation of a Module::FindFunction() variant that takes a CompilerContext, expesed via lldb-test.

rdar://120553412

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 5, 2024

@llvm/pr-subscribers-lldb

Author: Adrian Prantl (adrian-prantl)

Changes

…ntext

Following the specification chain seems to be clearly the expected behavior of GetDeclContext(). Otherwise C++ methods have an empty CompilerContext instead of being nested in their struct/class.

Theprimary motivation for this functionality is the Swift plugin. In order to test the change I added a proof-of-concept implementation of a Module::FindFunction() variant that takes a CompilerContext, expesed via lldb-test.

rdar://120553412


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

6 Files Affected:

  • (modified) lldb/include/lldb/Core/Module.h (+6)
  • (modified) lldb/source/Core/Module.cpp (+17)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp (+11-6)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+6-5)
  • (modified) lldb/test/Shell/SymbolFile/DWARF/x86/find-basic-function.cpp (+6)
  • (modified) lldb/tools/lldb-test/lldb-test.cpp (+4)
diff --git a/lldb/include/lldb/Core/Module.h b/lldb/include/lldb/Core/Module.h
index f4973cdda1efcc..0188057247a68b 100644
--- a/lldb/include/lldb/Core/Module.h
+++ b/lldb/include/lldb/Core/Module.h
@@ -337,6 +337,12 @@ class Module : public std::enable_shared_from_this<Module>,
                      const ModuleFunctionSearchOptions &options,
                      SymbolContextList &sc_list);
 
+  /// Find functions by compiler context.
+  void FindFunctions(llvm::ArrayRef<CompilerContext> compiler_ctx,
+                     lldb::FunctionNameType name_type_mask,
+                     const ModuleFunctionSearchOptions &options,
+                     SymbolContextList &sc_list);
+
   /// Find functions by name.
   ///
   /// If the function is an inlined function, it will have a block,
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index c0574b724ace7b..4d2fa18c55b1d7 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -855,6 +855,23 @@ void Module::FindFunctions(ConstString name,
   }
 }
 
+void Module::FindFunctions(llvm::ArrayRef<CompilerContext> compiler_ctx,
+                           FunctionNameType name_type_mask,
+                           const ModuleFunctionSearchOptions &options,
+                           SymbolContextList &sc_list) {
+  if (compiler_ctx.empty() ||
+      compiler_ctx.back().kind != CompilerContextKind::Function)
+    return;
+  ConstString name = compiler_ctx.back().name;
+  CompilerDeclContext dc;
+  SymbolContextList unfiltered;
+  FindFunctions(name, dc,name_type_mask, options, unfiltered);
+  // Filter by context.
+  for (auto sc : unfiltered)
+    if (sc.function && compiler_ctx.equals(sc.function->GetCompilerContext()))
+      sc_list.Append(sc);
+}
+
 void Module::FindFunctions(const RegularExpression &regex,
                            const ModuleFunctionSearchOptions &options,
                            SymbolContextList &sc_list) {
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
index bed68f45426f67..64211da37e7802 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
@@ -375,16 +375,21 @@ std::vector<DWARFDIE> DWARFDIE::GetDeclContextDIEs() const {
 
 std::vector<lldb_private::CompilerContext> DWARFDIE::GetDeclContext() const {
   std::vector<lldb_private::CompilerContext> context;
-  const dw_tag_t tag = Tag();
-  if (tag == DW_TAG_compile_unit || tag == DW_TAG_partial_unit)
-    return context;
   DWARFDIE parent = GetParent();
-  if (parent)
+  if (parent) {
+    const dw_tag_t parent_tag = parent.Tag();
+    if (parent_tag == DW_TAG_compile_unit || parent_tag == DW_TAG_partial_unit)
+      // Handle top-level DIEs by following the specification if any.
+      if (DWARFDIE spec = GetReferencedDIE(DW_AT_specification))
+        if (spec != *this)
+          return spec.GetDeclContext();
+
     context = parent.GetDeclContext();
+  }
   auto push_ctx = [&](CompilerContextKind kind, llvm::StringRef name) {
     context.push_back({kind, ConstString(name)});
   };
-  switch (tag) {
+  switch (Tag()) {
   case DW_TAG_module:
     push_ctx(CompilerContextKind::Module, GetName());
     break;
@@ -404,7 +409,7 @@ std::vector<lldb_private::CompilerContext> DWARFDIE::GetDeclContext() const {
     push_ctx(CompilerContextKind::Enum, GetName());
     break;
   case DW_TAG_subprogram:
-    push_ctx(CompilerContextKind::Function, GetPubname());
+    push_ctx(CompilerContextKind::Function, GetName());
     break;
   case DW_TAG_variable:
     push_ctx(CompilerContextKind::Variable, GetPubname());
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 447930ffe07b3f..f539ed5d2ae570 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2574,11 +2574,12 @@ void SymbolFileDWARF::FindFunctions(const Module::LookupInfo &lookup_info,
 
       Module::LookupInfo no_tp_lookup_info(lookup_info);
       no_tp_lookup_info.SetLookupName(ConstString(name_no_template_params));
-      m_index->GetFunctions(no_tp_lookup_info, *this, parent_decl_ctx, [&](DWARFDIE die) {
-        if (resolved_dies.insert(die.GetDIE()).second)
-          ResolveFunction(die, include_inlines, sc_list);
-        return true;
-      });
+      m_index->GetFunctions(no_tp_lookup_info, *this, parent_decl_ctx,
+                            [&](DWARFDIE die) {
+                              if (resolved_dies.insert(die.GetDIE()).second)
+                                ResolveFunction(die, include_inlines, sc_list);
+                              return true;
+                            });
     }
   }
 
diff --git a/lldb/test/Shell/SymbolFile/DWARF/x86/find-basic-function.cpp b/lldb/test/Shell/SymbolFile/DWARF/x86/find-basic-function.cpp
index 204568a446d0a4..30143a41d5e734 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/x86/find-basic-function.cpp
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/find-basic-function.cpp
@@ -34,6 +34,8 @@
 // RUN:   FileCheck --check-prefix=FULL-MANGLED-METHOD %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function --function-flags=base %t | \
 // RUN:   FileCheck --check-prefix=CONTEXT %s
+// RUN: lldb-test symbols --compiler-context=Struct:sbar,Function:foo -language=c++ -find=function -function-flags=method %t | \
+// RUN:   FileCheck --check-prefix=COMPILER-CONTEXT %s
 // RUN: lldb-test symbols --name=not_there --find=function %t | \
 // RUN:   FileCheck --check-prefix=EMPTY %s
 
@@ -84,6 +86,10 @@
 // CONTEXT: Found 1 functions:
 // CONTEXT-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv", decl_context = {Namespace(bar)}
 
+// COMPILER-CONTEXT: Found 2 functions:
+// COMPILER-CONTEXT-DAG: name = "sbar::foo()", mangled = "_ZN4sbar3fooEv"
+// COMPILER-CONTEXT-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
+
 // EMPTY: Found 0 functions:
 
 void foo() {}
diff --git a/lldb/tools/lldb-test/lldb-test.cpp b/lldb/tools/lldb-test/lldb-test.cpp
index e326a84c1dbd2f..33281cfb150747 100644
--- a/lldb/tools/lldb-test/lldb-test.cpp
+++ b/lldb/tools/lldb-test/lldb-test.cpp
@@ -466,6 +466,7 @@ static lldb::DescriptionLevel GetDescriptionLevel() {
 Error opts::symbols::findFunctions(lldb_private::Module &Module) {
   SymbolFile &Symfile = *Module.GetSymbolFile();
   SymbolContextList List;
+  auto compiler_context = parseCompilerContext();
   if (!File.empty()) {
     assert(Line != 0);
 
@@ -498,6 +499,9 @@ Error opts::symbols::findFunctions(lldb_private::Module &Module) {
     assert(RE.IsValid());
     List.Clear();
     Symfile.FindFunctions(RE, true, List);
+  } else if (!compiler_context.empty()) {
+    List.Clear();
+    Module.FindFunctions(compiler_context, getFunctionNameFlags(), {}, List);
   } else {
     Expected<CompilerDeclContext> ContextOr = getDeclContext(Symfile);
     if (!ContextOr)

Copy link

github-actions bot commented Jan 5, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

ResolveFunction(die, include_inlines, sc_list);
return true;
});
m_index->GetFunctions(no_tp_lookup_info, *this, parent_decl_ctx,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be just a formatting change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I missing something or is this an NFC clang-format change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I can't see any changes either

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is, the line was too long and I originally had a change here.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp Outdated Show resolved Hide resolved
lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@felipepiovezan felipepiovezan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw this reminds me a lot of accelerator tables & finding the parents of functions there.
Because the declaration is never placed in the accelerator table (and the declaration is what has the scope), we are never able to find the parent of functions with the accelerator tables.

We might be able to do a similar trick there as to what you are doing here

lldb/source/Core/Module.cpp Outdated Show resolved Hide resolved
lldb/source/Core/Module.cpp Outdated Show resolved Hide resolved
lldb/source/Core/Module.cpp Outdated Show resolved Hide resolved
lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp Outdated Show resolved Hide resolved
lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp Outdated Show resolved Hide resolved
…ntext

Following the specification chain seems to be clearly the expected
behavior of GetDeclContext(). Otherwise C++ methods have an empty
CompilerContext instead of being nested in their struct/class.

Theprimary motivation for this functionality is the Swift plugin. In
order to test the change I added a proof-of-concept implementation of
a Module::FindFunction() variant that takes a CompilerContext, expesed
via lldb-test.

rdar://120553412
Copy link
Member

@Michael137 Michael137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@adrian-prantl adrian-prantl merged commit fa92845 into llvm:main Jan 9, 2024
4 checks passed
adrian-prantl added a commit to adrian-prantl/llvm-project that referenced this pull request Jan 18, 2024
llvm#77157)

…ntext

Following the specification chain seems to be clearly the expected
behavior of GetDeclContext(). Otherwise C++ methods have an empty
CompilerContext instead of being nested in their struct/class.

Theprimary motivation for this functionality is the Swift plugin. In
order to test the change I added a proof-of-concept implementation of a
Module::FindFunction() variant that takes a CompilerContext, expesed via
lldb-test.

rdar://120553412
(cherry picked from commit fa92845)
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
llvm#77157)

…ntext

Following the specification chain seems to be clearly the expected
behavior of GetDeclContext(). Otherwise C++ methods have an empty
CompilerContext instead of being nested in their struct/class.

Theprimary motivation for this functionality is the Swift plugin. In
order to test the change I added a proof-of-concept implementation of a
Module::FindFunction() variant that takes a CompilerContext, expesed via
lldb-test.

rdar://120553412
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants