[lldb] Add support for looking up static const members

[werat]

Summary

Static const/constexpr members might have only DW_TAG_member entry with a
DW_AT_const_value attribute. Since there is no corresponding DW_TAG_variable
entry, these values are currently not indexed in the DWARF index and are not
available via SBTarget::FindGlobalVariables() API.

[teemperor]

Not sure who's the right person to review the SymbolFileDWARF.cpp/ManualDWARFIndex.cpp, but Pavel/Jan touched that not too long ago.

(D81471 might also be an interested patch for you. That one is just waiting on me to add some of the requested tests).

lldb/test/API/python_api/target/globals/TestTargetGlobals.py

6

Unused import

9

Also unused?

46

The code between here and self.build() can be simplified to:

 self.build()
target, _, _, _ lldbutil.run_to_source_breakpoint(self, "// Set a break at entry to main.", lldb.SBFileSpec("main.cpp"))

And then the setUp can also be removed.

FWIW, I believe we don't need to launch any process for this test, so this might be enough to test this (while being a much faster test):

self.build()
target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))

[werat]

Hi, please, take a look at this patch.

It seems Pavel already did some work for class-level static const variables in https://reviews.llvm.org/D86615, although that didn't work the case I've encountered.

In my example (which is also reflected in the test I've added), class-level static constexpr variable is presented in DWARF only via DW_TAG_member with DW_AT_const_value. The code on the lookup path looks only at DW_TAG_variable entries, which are not present.

[werat]

Simplified the test according to teemperor's comments.

[werat]

Remove unused field

[labath]

I am not sure if this is the right way to implement this feature. Changing ManualDWARFIndex to provide this additional information is easy enough, but it means that the other index classes will never be able to support this functionality (without, essentially, reimplementing ManualDWARFIndex and scanning all debug info). I am also worried about the increase to the manual index size, as this would mean that every occurrence of the DW_TAG_member would be placed in the index, whereas now we only place the one which has a location (which is normally just in a single compile unit).

I think it might be better to do something at the SymbolFileDWARF::FindGlobalVariables level, so that it is common to all indexes. For example, this function could (in case the normal search does not provide any information) try to strip the last component of the name (foo::bar::baz => foo::bar) and then try to look up the result as a class (type). If it finds the type, it could then check it for the specific (static) variable name.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3183

typo

(also "static constexpr" implies "static const" so maybe there's no need to explicitly mention the latter...)

3248

Why is this needed. I would expect this to evaluate to true even without this addition...

3270

Same here.

3518

Why is this needed? I wouldn't expect a member inside a function...

lldb/test/API/python_api/target/globals/TestTargetGlobals.py

15–17

delete

[werat]

Pavel, thanks for review!

In D92223#2422184, @labath wrote:

I am not sure if this is the right way to implement this feature. Changing ManualDWARFIndex to provide this additional information is easy enough, but it means that the other index classes will never be able to support this functionality (without, essentially, reimplementing ManualDWARFIndex and scanning all debug info).
...
I think it might be better to do something at the SymbolFileDWARF::FindGlobalVariables level, so that it is common to all indexes. For example, this function could (in case the normal search does not provide any information) try to strip the last component of the name (foo::bar::baz => foo::bar) and then try to look up the result as a class (type). If it finds the type, it could then check it for the specific (static) variable name.

This makes sense, let me try to implement this approach. A few questions before I jump into coding. Do you suggest to call m_index->GetTypes from SymbolFileDWARF::FindGlobalVariables and inspect the DIE for static members? Or use something more "high-level", e.g. SymbolFileDWARF::FindTypes and inspect the returned lldb_private::Type object? I had a brief look at Type and didn't find an easy way to get to static members there. Can you give me a pointer?

I am also worried about the increase to the manual index size, as this would mean that every occurrence of the DW_TAG_member would be placed in the index, whereas now we only place the one which has a location (which is normally just in a single compile unit).

Regarding the index size, we don't put ALL occurrences of DW_TAG_member in the index, only the ones with constant values. So it should be the same as if all these members had a location.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3248

In case of DW_TAG_member parent_tag is DW_TAG_class_type/DW_TAG_structure_type, so this check doesn't pass.

3270

As as above, parent_tag for DW_TAG_member is DW_TAG_class_type/DW_TAG_structure_type

3518

I'm not sure I understand, wdym by "member inside a function"? ParseVariables is called directly from FindGlobalVariables, so as far as I understand it this check applies for processing global variables as well.

[jankratochvil]

In D92223#2422184, @labath wrote:

I am not sure if this is the right way to implement this feature. Changing ManualDWARFIndex to provide this additional information is easy enough, but it means that the other index classes will never be able to support this functionality

I wanted to post here rather my agreement with the patch. This looks to me as giving up on the high performance index benefits. (But maybe what you say is the right sweet spot compromise.)

I rather find missing a feature to cross-check ManualDWARFIndex vs. on-disk indexes (lld/LTO produced .debug_names, gdb-add-index -dwarf-5 produced .debug_names and how Apple produces their index). Fedora is probably going to have two indexes (.gdb_index and .debug_names) for each LLVM-built binary as the format of GDB .debug_names is currently incompatible with LLVM/LLDB .debug_names. I also expect current ManualDWARFIndex is producing different index than lld.

I am also worried about the increase to the manual index size, as this would mean that every occurrence of the DW_TAG_member would be placed in the index, whereas now we only place the one which has a location (which is normally just in a single compile unit).

With -flto (even with -O0) there is only a single definition of each class.

[jankratochvil]

FYI it would not work with GNU C++ 4.4 (2008, RHEL-5) as it used DW_TAG_variable instead of DW_TAG_member. But that is probably not a problem, Red Hat's last supported RHEL for new toolchains is since RHEL-6 and for LLVM even just since RHEL-7:

<0><b>: Abbrev Number: 1 (DW_TAG_compile_unit)
    DW_AT_producer    : GNU C++ 4.1.2 20080704 (Red Hat 4.1.2-55)
    DW_AT_language    : 4      (C++)
    ...
<1><70>: Abbrev Number: 2 (DW_TAG_class_type)
    DW_AT_name        : C
    ...
<2><7a>: Abbrev Number: 3 (DW_TAG_variable)
    DW_AT_name        : i
    DW_AT_decl_file   : 1
    DW_AT_decl_line   : 2
    DW_AT_MIPS_linkage_name: _ZN1C1iE
    DW_AT_type        : <90>
    DW_AT_external    : 1
    DW_AT_accessibility: 3     (private)
    DW_AT_declaration : 1

[shafik]

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3181

This is a nitpick but if we move the comment above the if we can remove the braces.

[jankratochvil]

This patch may be superseded by D92643, not sure.

[werat]

In D92223#2546736, @jankratochvil wrote:

This patch may be superseded by D92643, not sure.

Yeah, D92643 is an alternative way to implement this, according to @labath comments. Sorry I didn't make it clear.

[jankratochvil]

Based on https://reviews.llvm.org/D92643#inline-925368 where the DIE traversal cannot performance-reasonably implement target variable -r I am returning here whether the .debug_names index could not be rather extended/improved. LLDB would fall back to ManualDWARFIndex if the on-disk index version is lower than what LLDB knows. For example GDB .gdb_index is now at version 8, GDB has also a use-deprecated-index-sections setting.

In D92223#2422184, @labath wrote:

I am also worried about the increase to the manual index size, as this would mean that every occurrence of the DW_TAG_member would be placed in the index, whereas now we only place the one which has a location (which is normally just in a single compile unit).

Only occurences which define (not declare) a static const member would be stored. I have measured it using https://people.redhat.com/jkratoch/lldb-staticconst.patch tested on a sample file bin/clang-13 being built -O0 -g -fno-lto using:

time cmake ../llvm-monorepo/llvm/ -DCMAKE_BUILD_TYPE=Debug -DLLVM_USE_LINKER=lld -DLLVM_ENABLE_PROJECTS="lldb;clang;lld" -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_OPTIMIZED_TABLEGEN=ON -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_USE_SPLIT_DWARF=OFF -DCMAKE_CXX_FLAGS="-gdwarf-5 -gpubnames" -DCMAKE_C_FLAGS="-gdwarf-5 -gpubnames" -GNinja;ninja ...;llvm-objcopy -R .debug_names bin/clang-13

And it increases the index size by 1.4% (by number of DW_IDX_die_offset entries, 14964676+210528). I find such increase worth the static const members effective access. The question is whether bin/clang-13 is a good sample binary.
That sure assumes an upstream DWARF discussion+update of the .debug_names format (and then there is the Apple index).

The static const members could be (based on a DWARF discussion) stored also only for one CU but I do not think it is needed given the size increase is not significant.

[jankratochvil]

This comment was removed by jankratochvil.

[jankratochvil]

The considered .debug_names index update would need to make .debug_names working in the first place - as it does not work now: D99850
So maybe the manual DIE traversal without updating the index and without the regex capability is OK but I do not want to give the agreement myself.

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

 //===-- ManualDWARFIndex.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/SymbolFile/DWARF/ManualDWARFIndex.h"
 #include "Plugins/Language/ObjC/ObjCLanguage.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugInfo.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDeclContext.h"
 #include "Plugins/SymbolFile/DWARF/LogChannelDWARF.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/Stream.h"
for (const DWARFDebugInfoEntry &die : unit.dies()) {
 
     switch (tag) {
     case DW_TAG_array_type:
     case DW_TAG_base_type:
     case DW_TAG_class_type:
     case DW_TAG_constant:
     case DW_TAG_enumeration_type:
     case DW_TAG_inlined_subroutine:
+    case DW_TAG_member:
     case DW_TAG_namespace:
     case DW_TAG_string_type:
     case DW_TAG_structure_type:
     case DW_TAG_subprogram:
     case DW_TAG_subroutine_type:
     case DW_TAG_typedef:
     case DW_TAG_union_type:
     case DW_TAG_unspecified_type:
case DW_TAG_unspecified_type:
         set.types.Insert(ConstString(mangled_cstr), ref);
       break;
 
     case DW_TAG_namespace:
       if (name)
         set.namespaces.Insert(ConstString(name), ref);
       break;
 
+    case DW_TAG_member:
     case DW_TAG_variable:
-      if (name && has_location_or_const_value && is_global_or_static_variable) {
+      if (name && has_location_or_const_value &&
+          (is_global_or_static_variable || tag == DW_TAG_member)) {
         set.globals.Insert(ConstString(name), ref);
         // Be sure to include variables by their mangled and demangled names if
         // they have any since a variable can have a basename "i", a mangled
         // named "_ZN12_GLOBAL__N_11iE" and a demangled mangled name
         // "(anonymous namespace)::i"...
 
         // Make sure our mangled name isn't the same string table entry as our
         // name. If it starts with '_', then it is ok, else compare the string

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

void SymbolFileDWARF::FindGlobalVariables(
   uint32_t pruned_idx = original_size;
 
   SymbolContext sc;
   m_index->GetGlobalVariables(ConstString(basename), [&](DWARFDIE die) {
     if (!sc.module_sp)
       sc.module_sp = m_objfile_sp->GetModule();
     assert(sc.module_sp);
 
-    if (die.Tag() != DW_TAG_variable)
+    // Look only for variables and members with static const data.
+    if (die.Tag() != DW_TAG_variable && die.Tag() != DW_TAG_member)
       return true;
 
     auto *dwarf_cu = llvm::dyn_cast<DWARFCompileUnit>(die.GetCU());
     if (!dwarf_cu)
       return true;
     sc.comp_unit = GetCompUnitForDWARFCompUnit(*dwarf_cu);
 
     if (parent_decl_ctx) {
VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
 
   if (VariableSP var_sp = GetDIEToVariable()[die.GetDIE()])
     return var_sp; // Already been parsed!
 
   const dw_tag_t tag = die.Tag();
   ModuleSP module = GetObjectFile()->GetModule();
 
   if (tag != DW_TAG_variable && tag != DW_TAG_constant &&
-      (tag != DW_TAG_formal_parameter || !sc.function))
+      tag != DW_TAG_member && (tag != DW_TAG_formal_parameter || !sc.function))
     return nullptr;
 
   DWARFAttributes attributes;
   const size_t num_attributes = die.GetAttributes(attributes);
   DWARFDIE spec_die;
   VariableSP var_sp;
   const char *name = nullptr;
   const char *mangled = nullptr;
for (size_t i = 0; i < num_attributes; ++i) {
     case DW_AT_visibility:
     default:
     case DW_AT_abstract_origin:
     case DW_AT_sibling:
       break;
     }
   }
 
+  if (tag == DW_TAG_member && !const_value_form.IsValid()) {
+    // Allows only members with DW_AT_const_value attribute, i.e. static const
+    // or static constexr.
+    return nullptr;
+  }
+
   // Prefer DW_AT_location over DW_AT_const_value. Both can be emitted e.g.
   // for static constexpr member variables -- DW_AT_const_value will be
   // present in the class declaration and DW_AT_location in the DIE defining
   // the member.
   bool location_is_const_value_data = false;
   bool has_explicit_location = false;
   bool use_type_size_for_value = false;
   if (location_form.IsValid()) {
if (DWARFFormValue::IsBlockForm(const_value_form.Form())) {
           DataExtractor(str, string_length, die.GetCU()->GetByteOrder(),
                         die.GetCU()->GetAddressByteSize()),
           die.GetCU());
     }
   }
 
   const DWARFDIE parent_context_die = GetDeclContextDIEContainingDIE(die);
   const dw_tag_t parent_tag = die.GetParent().Tag();
-  bool is_static_member = (parent_tag == DW_TAG_compile_unit ||
-                           parent_tag == DW_TAG_partial_unit) &&
-                          (parent_context_die.Tag() == DW_TAG_class_type ||
-                           parent_context_die.Tag() == DW_TAG_structure_type);
+  bool is_static_member =
+      (parent_tag == DW_TAG_compile_unit || parent_tag == DW_TAG_partial_unit ||
+       tag == DW_TAG_member) &&
 
   ValueType scope = eValueTypeInvalid;
 
   const DWARFDIE sc_parent_die = GetParentSymbolContextDIE(die);
   SymbolContextScope *symbol_context_scope = nullptr;
 
   bool has_explicit_mangled = mangled != nullptr;
   if (!mangled) {
     // LLDB relies on the mangled name (DW_TAG_linkage_name or
     // DW_AT_MIPS_linkage_name) to generate fully qualified names
     // of global variables with commands like "frame var j". For
     // example, if j were an int variable holding a value 4 and
     // declared in a namespace B which in turn is contained in a
     // namespace A, the command "frame var j" returns
     //   "(int) A::B::j = 4".
     // If the compiler does not emit a linkage name, we should be
     // able to generate a fully qualified name from the
     // declaration context.
     if ((parent_tag == DW_TAG_compile_unit ||
-         parent_tag == DW_TAG_partial_unit) &&
+         parent_tag == DW_TAG_partial_unit || tag == DW_TAG_member) &&
         Language::LanguageIsCPlusPlus(GetLanguage(*die.GetCU())))
       mangled =
           GetDWARFDeclContext(die).GetQualifiedNameAsConstString().GetCString();
   }
 
   if (tag == DW_TAG_formal_parameter)
     scope = eValueTypeVariableArgument;
   else {
if (is_static_lifetime) {
           } else {
             // Variable didn't make it into the final executable
             return var_sp;
           }
         }
       }
     } else {
       if (location_is_const_value_data &&
-          die.GetDIE()->IsGlobalOrStaticScopeVariable())
+          (die.GetDIE()->IsGlobalOrStaticScopeVariable() ||
+           tag == DW_TAG_member))
         scope = eValueTypeVariableStatic;
       else {
         scope = eValueTypeVariableLocal;
         if (debug_map_symfile) {
           // We need to check for TLS addresses that we need to fixup
           if (location.ContainsThreadLocalStorage()) {
             location.LinkThreadLocalStorage(
                 debug_map_symfile->GetObjectFile()->GetModule(),
while (die) {
     // Check to see if we have already parsed this variable or constant?
     VariableSP var_sp = GetDIEToVariable()[die.GetDIE()];
     if (var_sp) {
       if (cc_variable_list)
         cc_variable_list->AddVariableIfUnique(var_sp);
     } else {
       // We haven't already parsed it, lets do that now.
       if ((tag == DW_TAG_variable) || (tag == DW_TAG_constant) ||
+          (tag == DW_TAG_member) ||
           (tag == DW_TAG_formal_parameter && sc.function)) {
         if (variable_list_sp.get() == nullptr) {
           DWARFDIE sc_parent_die = GetParentSymbolContextDIE(orig_die);
           dw_tag_t parent_tag = sc_parent_die.Tag();
           switch (parent_tag) {
           case DW_TAG_compile_unit:
           case DW_TAG_partial_unit:
             if (sc.comp_unit != nullptr) {

lldb/test/API/python_api/target/globals/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/python_api/target/globals/TestTargetGlobals.py

+"""
+Test SBTarget::FindGlobalVariables API.
+"""
+
+import unittest2
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TargetAPITestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+
+    @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(
+            "Foo::static_constexpr_int",
+            "Foo::static_constexpr_int", "const int", "1")
+        test_global_var(
+            "Foo::static_constexpr_uint",
+            "Foo::static_constexpr_uint", "const unsigned int", "2")
+        test_global_var(
+            "Foo::static_const_out_out_class",
+            "Foo::static_const_out_out_class", "const int", "3")
+        test_global_var(
+            "global_var_of_char_type",
+            "::global_var_of_char_type", "char", "'X'")

lldb/test/API/python_api/target/globals/main.cpp

+struct Foo {
+  static constexpr int static_constexpr_int = 1;
+  static constexpr unsigned int static_constexpr_uint =
+      static_constexpr_int + 1;
+  static const int static_const_out_out_class;
+};
+
+const int Foo::static_const_out_out_class = 3;
+
+char global_var_of_char_type = 'X';
+
+int main() {
+  // Set a break at entry to main.
+  return 0;
+}