[lldb] Lookup static const members in FindGlobalVariables

[werat]

Summary

Static const members initialized inside a class definition might not have a corresponding DW_TAG_variable, 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.

[werat]

Hi, please, take a look at this patch!

This is an approach suggested by @labath in https://reviews.llvm.org/D92223#2422184

This approach feels a little bit "adhoc" (i.e. manually going through the DIE entries for const members), but it works :) Please, let me know if there's a better way to do this!

[labath]

In D92643#2433365, @werat wrote:

This approach feels a little bit "adhoc" (i.e. manually going through the DIE entries for const members), but it works :) Please, let me know if there's a better way to do this!

Are the static members included in the (SB)Type object that gets created when parsing the enclosing type? If yes, we might be able to retrieve them that way. Whether that would be cleaner -- I'm not sure...

(I would expect they are included, as otherwise they would be unavailable to the expression evaluator.)

[werat]

In D92643#2433428, @labath wrote:

Are the static members included in the (SB)Type object that gets created when parsing the enclosing type? If yes, we might be able to retrieve them that way. Whether that would be cleaner -- I'm not sure...

(I would expect they are included, as otherwise they would be unavailable to the expression evaluator.)

I think they're not as of now, but there's a patch from @teemperor to add them there -- D81471

Also, I was looking at Type/TypeSystem and it seems there's no API right now to get static members from a Type.

[jingham]

In D92643#2433441, @werat wrote:

In D92643#2433428, @labath wrote:

Are the static members included in the (SB)Type object that gets created when parsing the enclosing type? If yes, we might be able to retrieve them that way. Whether that would be cleaner -- I'm not sure...

(I would expect they are included, as otherwise they would be unavailable to the expression evaluator.)

I think they're not as of now, but there's a patch from @teemperor to add them there -- D81471

Also, I was looking at Type/TypeSystem and it seems there's no API right now to get static members from a Type.

I couldn't tell what you meant by this... I would expect that a Type would tell you that static members exist, and their type, etc. But I wouldn't expect a Type to be able to get a value for a static member. After all, the type comes from some Module, which could be shared amongst a bunch of processes in lldb. The actual member value could be different in each of these processes, but since the Type has no process, it has no way to get the correct value.

[werat]

In D92643#2434050, @jingham wrote:

I couldn't tell what you meant by this... I would expect that a Type would tell you that static members exist, and their type, etc. But I wouldn't expect a Type to be able to get a value for a static member. After all, the type comes from some Module, which could be shared amongst a bunch of processes in lldb. The actual member value could be different in each of these processes, but since the Type has no process, it has no way to get the correct value.

Yeah, of course you can't get a value for a static member just from the type (although, if it's an inline static then it could be possible?). But something like VariableList EnumerateStaticMembers(Target, Type) would be nice.

[werat]

@labath @teemperor gentle ping :) What do you think about this approach?

[labath]

It looks like the Type interface does not expose the static members (I guess it was never meant to do that), although, the underlying clang type obviously does contain them. After more consideration, I think parsing these straight from DWARF is fine, and consistent with how we parse other variables. I am wondering though if this should share the implementation with the existing parsing code (like your first patch did).

On a higher level, I am also worried about what this (the fact that we have to do this kind of a search) says about the quality/usefulness of the debug_names index. Overall, I seems to me that _not_ including these member variables in the index is the right call -- they would bloat the index (you'd have to put one of these next to every type declaration), and there are (as we've seen) other ways to find them.

I am not sure what to do about namespace-level constexpr variables, though... Currently, clang seems to put them into the index, even though that's not specified by standard. My guess is that this is by accident (it just inserts all global vars, without checking the formal inclusion rules). However, the question is what that means. If we were to consider this a bug and "fix" clang to stop emitting them, then we'd need to find a way to locate them in lldb. And doing this for namespaces won't be as easy/cheap as it is for classes. Maybe it doesn't have to block this patch (as I said, I think this part is ok), but I think it would be useful to discuss this with the people responsible for the generation of the index in clang and the wider dwarf community. (Unfortunately, I don't have the bandwidth to drive that dicussion :( ).

[werat]

Thanks for your review! If there are no objections to this patch, can you accept and land it for me? :)

In D92643#2445146, @labath wrote:

On a higher level, I am also worried about what this (the fact that we have to do this kind of a search) says about the quality/usefulness of the debug_names index.

FWIW we're considering disabling debug_names completely for now, as it degrades performance significantly with no extra benefit. We can discuss that in more details if you're interested.

In D92643#2445146, @labath wrote:

I think it would be useful to discuss this with the people responsible for the generation of the index in clang and the wider dwarf community. (Unfortunately, I don't have the bandwidth to drive that dicussion :( ).

Can't say I have a lot of expertise in DWARF, but I will try to research this more and reach out to clang/dwarf people.

[werat]

@labath @jankratochvil ping :)

[jankratochvil]

@werat I see you are aware of D81471. Do you or someone else have anything against it? I find it more integrated using the clang type for it but I admit I do not understand the clang integration much. At least it handles more cases, I stopped after finding:

$ echo 'struct Vars { static const int i = 2; } v; int main() {}'|./bin/clang -Wall -g -x c++ -;./bin/lldb -b ./a.out -o 'p Vars::i'
error: Can't evaluate the expression without a running target due to: Interpreter doesn't handle one of the expression's operands
vs. D81471's:
(const int) $0 = 2

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

3159

Here should be according to my D91014:

decl.SetFile(attributes.CompileUnitAtIndex(i)->GetFile(

[werat]

Thanks for reviewing!

In D92643#2546732, @jankratochvil wrote:

@werat I see you are aware of D81471. Do you or someone else have anything against it? I find it more integrated using the clang type for it but I admit I do not understand the clang integration much.

I don't have anything against it, on the contrary I'm all for it :) As a matter of fact, we've already integrated that patch in our internal LLDB fork a while ago.

But my patch is not a replacement for that one, it complements it by enhancing the SBTarget::FindGlobalVariables API.

[werat]

Change DW_AT_decl_file handling as per @jankratochvil comment.

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

3159

Thanks, didn't know about that

[jankratochvil]

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

2162

ConstString here is needlessly expensive to construct and it is then used only once. Use plain const char * or std::string is also much cheaper.

2162

This function also needs to be patched (with a testcase) as this command works:

(lldb) target variable Vars::inline_static
(double) Vars::inline_static = 1.5

But this one does not (and it should work):

(lldb) target variable -r Vars::inline_static
error: can't find global variable 'Vars::inline_static'

2166

Could this case have a testcase so that it cannot find such false positives?

3128

Could you try to merge this functionality into SymbolFileDWARF::ParseVariableDIE? There is now some code duplication. I hope the merge will not become too ugly. (@labath was also suggesting this.)

[werat]

Address review comments:

• Don't create expensive ConstString objects
• Merge ParseStaticConstMemberDIE into ParseVariableDIE
• Add more test cases

[werat]

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

2162

With the current approach implementing a search by regexp would mean enumerating ALL types and looking at ALL their static members for a potential match. This sounds way to expensive (and a significant degradation compared to the current logic). On the other hand I agree that's very confusing that target variable works, but target variable -r doesn't. What do you think?

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

while (pruned_idx < variables.GetSize()) {
         ++pruned_idx;
       else
         variables.RemoveVariableAtIndex(pruned_idx);
     }
 
     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_scope;
+    llvm::StringRef type_name;
+    TypeClass type_class;
+    if (!Type::GetTypeScopeAndBasename(context, type_scope, type_name,
+                                       type_class))
+      type_name = context;
+
+    m_index->GetTypes(ConstString(type_name), [&](DWARFDIE parent) {
+      DWARFDeclContext dwarf_decl_ctx = GetDWARFDeclContext(parent);
+      llvm::StringRef parent_type_name = dwarf_decl_ctx.GetQualifiedName();
+
+      // This type is from another scope, skip it.
+      if (!parent_type_name.endswith(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 = ParseVariableDIE(sc, die, LLDB_INVALID_ADDRESS)) {
+          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) {
     GetObjectFile()->GetModule()->LogMessage(
         log,
         "SymbolFileDWARF::FindGlobalVariables (name=\"%s\", "
         "parent_decl_ctx=%p, max_matches=%u, variables) => %u",
         name.GetCString(), static_cast<const void *>(&parent_decl_ctx),
         max_matches, num_matches);
   }
 }
 
 void SymbolFileDWARF::FindGlobalVariables(const RegularExpression &regex,
                                           uint32_t max_matches,
                                           VariableList &variables) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   Log *log(LogChannelDWARF::GetLogIfAll(DWARF_LOG_LOOKUPS));
 
   if (log) {
     GetObjectFile()->GetModule()->LogMessage(
         log,
if (sc.function) {
             });
       }
       return vars_added;
     }
   }
   return 0;
 }
 
 VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
                                              const DWARFDIE &die,
                                              const lldb::addr_t func_low_pc) {
   if (die.GetDWARF() != this)
     return die.GetDWARF()->ParseVariableDIE(sc, die, func_low_pc);
 
   if (!die)
     return nullptr;
 
   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;
   Declaration decl;
   DWARFFormValue type_die_form;
   DWARFExpression location;
   bool is_external = false;
   bool is_artificial = false;
   DWARFFormValue const_value_form, location_form;
   Variable::RangeList scope_ranges;
 
   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:
for (size_t i = 0; i < num_attributes; ++i) {
     case DW_AT_visibility:
     default:
     case DW_AT_abstract_origin:
     case DW_AT_sibling:
       break;
     }
   }
 
+  // Look only for static const members with const values.
+  if (tag == DW_TAG_member &&
+      !DWARFFormValue::IsDataForm(const_value_form.Form()))
+    return nullptr;
+
+  const dw_tag_t parent_tag = die.GetParent().Tag();
+  ModuleSP module = GetObjectFile()->GetModule();
+
   // 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())) {
       location = DWARFExpression(
           module,
           DataExtractor(str, string_length, die.GetCU()->GetByteOrder(),
                         die.GetCU()->GetAddressByteSize()),
           die.GetCU());
     }
   }
 
+  // DW_TAG_member entries are obviously static members in this context.
+  bool is_static_member = tag == DW_TAG_member;
+  // Other entries can be static members too, look at their parent.
+  if (!is_static_member) {
     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);
-
-  ValueType scope = eValueTypeInvalid;
-
-  const DWARFDIE sc_parent_die = GetParentSymbolContextDIE(die);
-  SymbolContextScope *symbol_context_scope = nullptr;
+    is_static_member = (parent_tag == DW_TAG_compile_unit ||
+  }
 
   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 ||
+    if ((tag == DW_TAG_member || parent_tag == DW_TAG_compile_unit ||
          parent_tag == DW_TAG_partial_unit) &&
         Language::LanguageIsCPlusPlus(GetLanguage(*die.GetCU())))
       mangled =
           GetDWARFDeclContext(die).GetQualifiedNameAsConstString().GetCString();
   }
 
-  if (tag == DW_TAG_formal_parameter)
+  ValueType scope = eValueTypeInvalid;
+  SymbolContextScope *symbol_context_scope = nullptr;
+
+  if (tag == DW_TAG_member)
+    scope = eValueTypeVariableGlobal;
+  else if (tag == DW_TAG_formal_parameter)
     scope = eValueTypeVariableArgument;
   else {
     // DWARF doesn't specify if a DW_TAG_variable is a local, global
     // or static variable, so we have to do a little digging:
     // 1) DW_AT_linkage_name implies static lifetime (but may be missing)
     // 2) An empty DW_AT_location is an (optimized-out) static lifetime var.
     // 3) DW_AT_location containing a DW_OP_addr implies static lifetime.
     // Clang likes to combine small global variables into the same symbol
VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
   }
 
   if (symbol_context_scope == nullptr) {
     switch (parent_tag) {
     case DW_TAG_subprogram:
     case DW_TAG_inlined_subroutine:
     case DW_TAG_lexical_block:
       if (sc.function) {
+        const DWARFDIE sc_parent_die = GetParentSymbolContextDIE(die);
         symbol_context_scope =
             sc.function->GetBlock(true).FindBlockByID(sc_parent_die.GetID());
         if (symbol_context_scope == nullptr)
           symbol_context_scope = sc.function;
       }
       break;
 
     default:

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.
+"""
+
+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, 100500)
+            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'")
+
+        # Looking for a specific scope should find the variable from that scope.
+        test_global_var("ns1::Klass::x", "ns1::Klass::x", "int", "1")
+        test_global_var("ns2::Klass::x", "ns2::Klass::x", "int", "2")
+        
+        # However the query for `Klass::x` should return both variables:
+        # `ns1::Klass::x` and `ns2::Klass::x`.
+        values = target.FindGlobalVariables("Klass::x", 100500)
+        self.assertEqual(values.GetSize(), 2)
+        self.assertEqual(
+            {values[0].GetName(), values[1].GetName()}, 
+            {"ns1::Klass::x", "ns2::Klass::x"})

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

+namespace ns1 {
+class Klass {
+public:
+  inline static int x = 1;
+};
+} // namespace ns1
+
+namespace ns2 {
+class Klass {
+public:
+  inline static int x = 2;
+};
+} // namespace ns2
+
+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() {
+  ns1::Klass ns1;
+  ns2::Klass ns2;
+  Vars v;
+}