[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

3162

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

3162

Thanks, didn't know about that

[jankratochvil]

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

2166

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.

2166

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'

2170

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

3131

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

2166

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.h

 #include <mutex>
 #include <unordered_map>
 #include <vector>
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/Support/Threading.h"
 
 #include "lldb/Core/UniqueCStringMap.h"
 #include "lldb/Core/dwarf.h"
 #include "lldb/Symbol/DebugMacros.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Flags.h"
 #include "lldb/Utility/RangeMap.h"
 #include "lldb/lldb-private.h"
protected:
   lldb::TypeSP ParseType(const lldb_private::SymbolContext &sc,
                          const DWARFDIE &die, bool *type_is_new);
 
   lldb_private::Type *ResolveTypeUID(const DWARFDIE &die,
                                      bool assert_not_being_parsed);
 
   lldb_private::Type *ResolveTypeUID(const DIERef &die_ref);
 
+  lldb::VariableSP
+  ParseStaticConstMemberDIE(const lldb_private::SymbolContext &sc,
+                            const DWARFDIE &die);
+
   lldb::VariableSP ParseVariableDIE(const lldb_private::SymbolContext &sc,
                                     const DWARFDIE &die,
                                     const lldb::addr_t func_low_pc);
 
   size_t ParseVariables(const lldb_private::SymbolContext &sc,
                         const DWARFDIE &orig_die,
                         const lldb::addr_t func_low_pc, bool parse_siblings,
                         bool parse_children,

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) {
+      llvm::StringRef parent_type_name = GetDWARFDeclContext(parent)
+                                             .GetQualifiedNameAsConstString()
+                                             .GetStringRef();
+
+      // 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 = 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) {
     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::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;
+  const size_t num_attributes = die.GetAttributes(attributes);
+
+  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;
+    }
+  }
+
+  // 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, GetUID(type_die_form.Reference()));
+
+  if (type_sp->GetType())
+    location.UpdateValue(const_value_form.Unsigned(),
+                         type_sp->GetType()->GetByteSize(nullptr).getValueOr(0),
+                         die.GetCU()->GetAddressByteSize());
+
+  if (Language::LanguageIsCPlusPlus(GetLanguage(*die.GetCU())))
+    mangled =
+        GetDWARFDeclContext(die).GetQualifiedNameAsConstString().GetCString();
+
+  ValueType scope = eValueTypeVariableGlobal;
+  Variable::RangeList scope_ranges;
+
+  VariableSP var_sp = std::make_shared<Variable>(
+      die.GetID(), name, mangled, type_sp, scope, sc.comp_unit, scope_ranges,
+      &decl, location, /*is_external*/ true, /*is_artificial*/ false,
+      /*location_is_const_value_data*/ true,
+      /*is_static_member*/ true);
+
+  // Cache this variable, so we don't parse it over and over again.
+  GetDIEToVariable()[die.GetDIE()] = var_sp;
+
+  return var_sp;
+}
+
 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;

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, 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'")

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

+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() {}