llvm-project icon indicating copy to clipboard operation
llvm-project copied to clipboard

[lldb] Lookup static const members in FindGlobalVariables

Open kuilpd opened this issue 1 year ago • 8 comments

Static const members initialized inside a class definition might not have a corresponding DW_TAG_variable (DWARF 4 and earlier), so they're not indexed by ManualDWARFIndex.

Add an additional lookup in FindGlobalVariables. Try looking up the enclosing type (e.g. foo::bar for foo::bar::A) and then searching for a static const member (A) within this type.

kuilpd avatar Oct 10 '24 15:10 kuilpd

@llvm/pr-subscribers-lldb

Author: Ilia Kuklin (kuilpd)

Changes

Static const members initialized inside a class definition might not have a corresponding DW_TAG_variable (DWARF 4 and earlier), so they're not indexed by ManualDWARFIndex.

Add an additional lookup in FindGlobalVariables. Try looking up the enclosing type (e.g. foo::bar for foo::bar::A) and then searching for a static const member (A) within this type.


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

5 Files Affected:

  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+129)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h (+4)
  • (added) lldb/test/API/python_api/frame/globals/Makefile (+4)
  • (added) lldb/test/API/python_api/frame/globals/TestTargetGlobals.py (+42)
  • (added) lldb/test/API/python_api/frame/globals/main.cpp (+12)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 9287d4baf19e9c..f5a68a9c46c509 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2439,6 +2439,48 @@ void SymbolFileDWARF::FindGlobalVariables(
     return variables.GetSize() - original_size < max_matches;
   });
 
+  // If we don't have enough matches and the variable context is not empty, try
+  // to resolve the context as a type and look for static const members.
+  if (variables.GetSize() - original_size < max_matches && !context.empty()) {
+    llvm::StringRef type_name;
+    if (std::optional<Type::ParsedName> parsed_name =
+        Type::GetTypeScopeAndBasename(context))
+      type_name = parsed_name->basename;
+    else
+      type_name = context;
+
+    m_index->GetTypes(ConstString(type_name), [&](DWARFDIE parent) {
+      llvm::StringRef parent_type_name = parent.GetDWARFDeclContext()
+                                             .GetQualifiedNameAsConstString()
+                                             .GetStringRef();
+
+      // This type is from another scope, skip it.
+      if (!parent_type_name.ends_with(context))
+        return true;
+
+      auto *dwarf_cu = llvm::dyn_cast<DWARFCompileUnit>(parent.GetCU());
+      if (!dwarf_cu)
+        return true;
+
+      sc.comp_unit = GetCompUnitForDWARFCompUnit(*dwarf_cu);
+
+      for (DWARFDIE die = parent.GetFirstChild(); die.IsValid();
+           die = die.GetSibling()) {
+        // Try parsing the entry as a static const member.
+        if (auto var_sp = ParseStaticConstMemberDIE(sc, die)) {
+          if (var_sp->GetUnqualifiedName().GetStringRef() != basename)
+            continue;
+
+          // There can be only one member with a given name.
+          variables.AddVariableIfUnique(var_sp);
+          break;
+        }
+      }
+
+      return variables.GetSize() - original_size < max_matches;
+    });
+  }
+
   // Return the number of variable that were appended to the list
   const uint32_t num_matches = variables.GetSize() - original_size;
   if (log && num_matches > 0) {
@@ -3371,6 +3413,93 @@ size_t SymbolFileDWARF::ParseVariablesForContext(const SymbolContext &sc) {
   return 0;
 }
 
+VariableSP SymbolFileDWARF::ParseStaticConstMemberDIE(
+    const lldb_private::SymbolContext &sc, const DWARFDIE &die) {
+  if (die.GetDWARF() != this)
+    return die.GetDWARF()->ParseStaticConstMemberDIE(sc, die);
+
+  // Look only for members, ignore all other types of entries.
+  if (die.Tag() != DW_TAG_member)
+    return nullptr;
+
+  if (VariableSP var_sp = GetDIEToVariable()[die.GetDIE()])
+    return var_sp; // Already been parsed!
+
+  const char *name = nullptr;
+  const char *mangled = nullptr;
+  Declaration decl;
+  DWARFExpression location;
+  DWARFFormValue type_die_form;
+  DWARFFormValue const_value_form;
+
+  DWARFAttributes attributes = die.GetAttributes();
+  const size_t num_attributes = attributes.Size();
+
+  for (size_t i = 0; i < num_attributes; ++i) {
+    dw_attr_t attr = attributes.AttributeAtIndex(i);
+    DWARFFormValue form_value;
+
+    if (!attributes.ExtractFormValueAtIndex(i, form_value))
+      continue;
+
+    switch (attr) {
+    case DW_AT_decl_file:
+      decl.SetFile(sc.comp_unit->GetSupportFiles().GetFileSpecAtIndex(
+          form_value.Unsigned()));
+      break;
+    case DW_AT_decl_line:
+      decl.SetLine(form_value.Unsigned());
+      break;
+    case DW_AT_decl_column:
+      decl.SetColumn(form_value.Unsigned());
+      break;
+    case DW_AT_name:
+      name = form_value.AsCString();
+      break;
+    case DW_AT_type:
+     type_die_form = form_value;
+      break;
+    case DW_AT_const_value:
+      const_value_form = form_value;
+      break;
+    default:
+      break;
+    }
+  }
+
+  // Look only for static const members with const values.
+  if (!DWARFFormValue::IsDataForm(const_value_form.Form()))
+    return nullptr;
+
+  SymbolFileTypeSP type_sp = std::make_shared<SymbolFileType>(
+     *this, type_die_form.Reference().GetID());
+
+  if (type_sp->GetType()) {
+    location.UpdateValue(const_value_form.Unsigned(),
+                         type_sp->GetType()->GetByteSize(nullptr).value_or(0),
+                         die.GetCU()->GetAddressByteSize());
+  }
+
+  if (Language::LanguageIsCPlusPlus(GetLanguage(*die.GetCU())))
+    mangled =
+        die.GetDWARFDeclContext().GetQualifiedNameAsConstString().GetCString();
+
+  ValueType scope = eValueTypeVariableGlobal;
+  Variable::RangeList scope_ranges;
+
+  DWARFExpressionList location_list(GetObjectFile()->GetModule(), location, die.GetCU());
+  VariableSP var_sp = std::make_shared<Variable>(
+      die.GetID(), name, mangled, type_sp, scope, sc.comp_unit, scope_ranges,
+      &decl, location_list, /*is_external*/ true, /*is_artificial*/ false,
+      /*is_static_member*/ true);
+  var_sp->SetLocationIsConstantValueData(true);
+
+  // Cache this variable, so we don't parse it over and over again.
+  GetDIEToVariable()[die.GetDIE()] = var_sp;
+
+  return var_sp;
+}
+
 VariableSP SymbolFileDWARF::ParseVariableDIECached(const SymbolContext &sc,
                                                    const DWARFDIE &die) {
   if (!die)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
index 4967b37d753a09..0565dd0a666826 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -408,6 +408,10 @@ class SymbolFileDWARF : public SymbolFileCommon {
   bool ParseSupportFiles(DWARFUnit &dwarf_cu, const lldb::ModuleSP &module,
                          SupportFileList &support_files);
 
+  lldb::VariableSP
+  ParseStaticConstMemberDIE(const SymbolContext &sc,
+                            const DWARFDIE &die);
+
   lldb::VariableSP ParseVariableDIE(const SymbolContext &sc,
                                     const DWARFDIE &die,
                                     const lldb::addr_t func_low_pc);
diff --git a/lldb/test/API/python_api/frame/globals/Makefile b/lldb/test/API/python_api/frame/globals/Makefile
new file mode 100644
index 00000000000000..c24259ec8c9d93
--- /dev/null
+++ b/lldb/test/API/python_api/frame/globals/Makefile
@@ -0,0 +1,4 @@
+CXX_SOURCES := main.cpp
+CXXFLAGS_EXTRAS := -gdwarf-4
+
+include Makefile.rules
diff --git a/lldb/test/API/python_api/frame/globals/TestTargetGlobals.py b/lldb/test/API/python_api/frame/globals/TestTargetGlobals.py
new file mode 100644
index 00000000000000..edbd97ceadaf67
--- /dev/null
+++ b/lldb/test/API/python_api/frame/globals/TestTargetGlobals.py
@@ -0,0 +1,42 @@
+"""
+Test SBTarget::FindGlobalVariables API.
+"""
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+class TargetAPITestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @add_test_categories(['pyapi'])
+    def test_find_global_variables(self):
+        """Exercise SBTarget.FindGlobalVariables() API."""
+        self.build()
+
+        # Don't need to launch a process, since we're only interested in
+        # looking up global variables.
+        target = self.dbg.CreateTarget(self.getBuildArtifact())
+
+        def test_global_var(query, name, type_name, value):
+            value_list = target.FindGlobalVariables(query, 1)
+            self.assertEqual(value_list.GetSize(), 1)
+            var = value_list.GetValueAtIndex(0)
+            self.DebugSBValue(var)
+            self.assertTrue(var)
+            self.assertEqual(var.GetName(), name)
+            self.assertEqual(var.GetTypeName(), type_name)
+            self.assertEqual(var.GetValue(), value)
+
+        test_global_var(
+            "Vars::inline_static",
+            "Vars::inline_static", "double", "1.5")
+        test_global_var(
+            "Vars::static_constexpr",
+            "Vars::static_constexpr", "const int", "2")
+        test_global_var(
+            "Vars::static_const_out_out_class",
+            "Vars::static_const_out_out_class", "const int", "3")
+        test_global_var(
+            "global_var_of_char_type",
+            "::global_var_of_char_type", "char", "'X'")
diff --git a/lldb/test/API/python_api/frame/globals/main.cpp b/lldb/test/API/python_api/frame/globals/main.cpp
new file mode 100644
index 00000000000000..e2095e80082677
--- /dev/null
+++ b/lldb/test/API/python_api/frame/globals/main.cpp
@@ -0,0 +1,12 @@
+class Vars {
+public:
+  inline static double inline_static = 1.5;
+  static constexpr int static_constexpr = 2;
+  static const int static_const_out_out_class;
+};
+
+const int Vars::static_const_out_out_class = 3;
+
+char global_var_of_char_type = 'X';
+
+int main() {}

llvmbot avatar Oct 10 '24 15:10 llvmbot

This patch aims to retrieve static const members specifically from debug information versions DWARF 4 and earlier, where such members are marked DW_TAG_member. In DWARF 5 they are marked DW_TAG_variable and get retrieved normally without this patch. This was initially created to retrieve debug info in Unreal Engine, which still uses DWARF 4 on Linux: https://reviews.llvm.org/D92643

Right now there is a data formatter that has to use expression evaluation to recalculate something that have already been done during compile time, but it cannot access it because it's a static constexpr member of a struct.

https://github.com/EpicGames/UnrealEngine/blob/40eea367040d50aadd9f030ed5909fc890c159c2/Engine/Extras/LLDBDataFormatters/UEDataFormatters_2ByteChars.py#L84

kuilpd avatar Oct 10 '24 15:10 kuilpd

:white_check_mark: With the latest revision this PR passed the Python code formatter.

github-actions[bot] avatar Oct 10 '24 15:10 github-actions[bot]

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

github-actions[bot] avatar Oct 10 '24 15:10 github-actions[bot]

I think we may want to implement this in a slightly different matter: find the unique definition DIE for the given type (we use that concept when parsing types) and then search for child inside that -- instead of searching through all (possibly one per CU) definitions of that type.

I am a bit confused here, don't we need to look through all the debug information to find the type anyway? Could you please point me to the code with type parsing you are referring to?

However, before we go down that path, I want to note that something like what you want is already possible using the python API:

(lldb) target var Vars::static_constexpr
error: can't find global variable 'Vars::static_constexpr'
(lldb) script lldb.target.FindFirstType("Vars").GetStaticFieldWithName("static_constexpr").GetConstantValue(lldb.target)
(const int) static_constexpr = 2

The custom formatter's expression is an example of how I found this bug, but I think it should be fixed regardless, this function can be used from API in other circumstances.

It would still be nice if the "target variable" command worked as well, but:

  • I'm a little less concerned about that given that it works for dwarf v5 (clang default)

  • you should be using the python API (instead of expression evaluation) in the data formatter anyway, as it's faster and more reliable

Would checking the DWARF version and doing the search only on v4 and earlier improve the situation somewhat?

kuilpd avatar Oct 16 '24 19:10 kuilpd

I think we may want to implement this in a slightly different matter: find the unique definition DIE for the given type (we use that concept when parsing types) and then search for child inside that -- instead of searching through all (possibly one per CU) definitions of that type.

I am a bit confused here, don't we need to look through all the debug information to find the type anyway? Could you please point me to the code with type parsing you are referring to?

Not really. You can stop the as soon as you find the first definition of that type (even though the definition may be copied into every compile unit out there.

The code I'm referring to is the UniqueDWARFASTTypeMap class (and the code surrounding it). We use that to determine whether to DIEs describe the same type. My idea was something like: a) look up the type that is supposed to contain the variable we're searching for (using FindTypes or something like that); b) get the DWARF DIE describing that type; c) look at the children of that DIE for the variable

However, before we go down that path, I want to note that something like what you want is already possible using the python API:

(lldb) target var Vars::static_constexpr
error: can't find global variable 'Vars::static_constexpr'
(lldb) script lldb.target.FindFirstType("Vars").GetStaticFieldWithName("static_constexpr").GetConstantValue(lldb.target)
(const int) static_constexpr = 2

The custom formatter's expression is an example of how I found this bug, but I think it should be fixed regardless, this function can be used from API in other circumstances.

I agree. It's just that I think this has a slightly lower priority because the problem will presumably "fix" itself over time as people migrate to the new version. I was mentioning this in case your primary (only?) motivation is to fix the data formatter, as that approach would be faster (in any DWARF version).

It would still be nice if the "target variable" command worked as well, but:

  • I'm a little less concerned about that given that it works for dwarf v5 (clang default)
  • you should be using the python API (instead of expression evaluation) in the data formatter anyway, as it's faster and more reliable

Would checking the DWARF version and doing the search only on v4 and earlier improve the situation somewhat?

Somewhat, yes, but I'm not sure how we'd go about this, as every compile unit in a file can be compiled with a different version (and you won't know which one it is until you actually search for it).

It might actually be better to fix this problem during indexing, in the ManualDWARFIndex -- basically, if we're indexing a v4 unit and we run into a static const(expr?) variable (can we detect that reliably by looking at the DIE alone?), then we add it to the index, just as if it was a v5 DW_TAG_variable.

I'm normally want to avoid solving lookup issues by modifying the contents of the index (as that means things behave differently depending on which index you're using), but since in this case we, in a way, bringing the manual index in line with the compiler-generated debug names index (and debug_names does not support DWARF v4), I think this would be okay.

labath avatar Oct 18 '24 11:10 labath

The code I'm referring to is the UniqueDWARFASTTypeMap class (and the code surrounding it). We use that to determine whether to DIEs describe the same type. My idea was something like: a) look up the type that is supposed to contain the variable we're searching for (using FindTypes or something like that); b) get the DWARF DIE describing that type; c) look at the children of that DIE for the variable

Thank you, I will look into it.

It might actually be better to fix this problem during indexing, in the ManualDWARFIndex -- basically, if we're indexing a v4 unit and we run into a static const(expr?) variable (can we detect that reliably by looking at the DIE alone?), then we add it to the index, just as if it was a v5 DW_TAG_variable.

This sounds like the best solution indeed. I will try doing this first and see if it's possible at all to detect such a case. However, there might be other cases where global variables are not tagged as DW_TAG_variable, but to be honest I don't know any other example other than static constexpr anyway.

kuilpd avatar Oct 18 '24 12:10 kuilpd

It might actually be better to fix this problem during indexing, in the ManualDWARFIndex -- basically, if we're indexing a v4 unit and we run into a static const(expr?) variable (can we detect that reliably by looking at the DIE alone?), then we add it to the index, just as if it was a v5 DW_TAG_variable.

Implemented this as best as I could understand from the DWARF specs, but not sure if there are some cases where the checks I made can be false positive.

Could we re-use TestConstStaticIntegralMember.py? Surprised it doesn't already have the tests added here XFAILed

Thank you, I expanded this test instead of making a new one.

kuilpd avatar Oct 24 '24 18:10 kuilpd

I've disabled the new tests on Windows but can't look into the reason for the failures until next week.

DavidSpickett avatar Nov 07 '24 15:11 DavidSpickett

This also fails on the macOS buildbots: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/14840/execution/node/97/log/

FAIL: test_inline_static_members_dwarf4_dsym (TestConstStaticIntegralMember.TestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1769, in test_method
    return attrvalue(self)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake/llvm-project/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py", line 153, in test_inline_static_members_dwarf4
    self.check_inline_static_members("-gdwarf-4")
  File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake/llvm-project/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py", line 129, in check_inline_static_members
    self.check_global_var("A::int_val", "const int", "1")
  File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake/llvm-project/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py", line 118, in check_global_var
    self.assertGreaterEqual(len(var_list), 1)
AssertionError: 0 not greater than or equal to 1

Could you take a look? Not sure why the XFAIL isn't catching this.

Michael137 avatar Nov 07 '24 15:11 Michael137

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

Michael137 avatar Nov 08 '24 00:11 Michael137