[lldb] Fix lookup for global constants in namespaces

[tonkosi]

Summary

LLDB uses mangled name to construct a fully qualified name for global
variables. Sometimes DW_TAG_linkage_name attribute is missing from
debug info, so LLDB has to rely on parent entries to construct the
fully qualified name.

Currently, the fallback is handled when the parent DW_TAG is either
DW_TAG_compiled_unit or DW_TAG_partial_unit, which may not work well
for global constants in namespaces. For example:

namespace ns {
  const int x = 10;
}

may produce the following debug info:

<1><2a>: Abbrev Number: 2 (DW_TAG_namespace)
   <2b>   DW_AT_name        : (indirect string, offset: 0x5e): ns
<2><2f>: Abbrev Number: 3 (DW_TAG_variable)
   <30>   DW_AT_name        : (indirect string, offset: 0x61): x
   <34>   DW_AT_type        : <0x3c>
   <38>   DW_AT_decl_file   : 1
   <39>   DW_AT_decl_line   : 2
   <3a>   DW_AT_const_value : 10

Since the fallback didn't handle the case when parent tag is
DW_TAG_namespace, LLDB wasn't able to match the variable by its fully
qualified name "ns::x". This change fixes this by additional check
if the parent is a DW_TAG_namespace.

[werat]

This looks reasonable to me.

@teemperor any thoughts?

[teemperor]

LGTM, but I have never touched this function so +Greg and Jan.

[clayborg]

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

3281–3282

I think we should just always call this function in regardless of what the parent variable tag is since what happens if the parent tag is another value decl context like DW_TAG_class_type, DW_TAG_structure_type?

The call to GetDWARFDeclContext(die) below will calculate the DWARFDeclContext for a DIE and then construct an appropriate qualified name, so we can just always do this so I would suggest just making this:

if (Language::LanguageIsCPlusPlus(GetLanguage(*die.GetCU())))
  mangled = GetDWARFDeclContext(die).GetQualifiedNameAsConstString().GetCString();

Then we should always get this right all the time. It doesn't actually make sense to call this if the parent is the DW_TAG_compile_unit or DW_TAG_partial_unit because then there is no decl context to add to the variable name.

lldb/test/API/lang/cpp/global_variables/main.cpp

5

might be nice to see if we can create constants inside of a class or struct as well as mentioned in the above inline comment

[tonkosi]

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

3281–3282

I tried to call GetDWARFDeclContext(die) in a general case, but it seems that GetQualifiedName will append :: (source) in front of the variable name if there's no other declaration context.

As a result, LLDB will display local variables and function arguments as ::var. Any suggestion to deal with that?

I also noticed that with the current change it would be possible to get variables in anonymous namespaces, e.g. for the following source

namespace ns {
namespace {
const int var = 1;
}
}

call to SBTarget::FindGlobalVariables("ns::(anonymous namespace)::var") will succeed. Is that OK?

lldb/test/API/lang/cpp/global_variables/main.cpp

5

I couldn't come up with a struct or class example that triggers changes in this diff. Members were either DW_TAG_member which is not handled by ParseVariableDIE or they already contained DW_AT_linkage_name.

[clayborg]

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

3281–3282

GetQualifiedNameAsConstString could take an extra argument that specifies if we should prepend the "::" to the root namespace.

I am fine with a lookup of "ns::(anonymous namespace)::var" succeeding.

lldb/test/API/lang/cpp/global_variables/main.cpp

5

ok, thanks for trying

[shafik]

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

3281–3282

You should document the anonymous namespace case in a test.

[tonkosi]

• Use scope context parent DIE instead of the direct parent
• Add tests for constants in anonymous namespace

[tonkosi]

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

3281–3282

Seems that prepended :: was not the only issue, local variables and arguments that were part of functions in a namespace were also formatted as ns::local_var. So my conclusion was that this check shouldn't be performed for local scope anyway.

I noticed there's a variable sc_parent_die which gives the first parent that is one of these: DW_TAG_compile_unit, DW_TAG_partial_unit, DW_TAG_subprogram, DW_TAG_inline_subroutine, DW_TAG_lexical_block. Since the last three probably refer to local scope, I thought it would make sense to check compile/partial unit for global scope (could it be that sc_parent_die was meant to be used originally instead of the direct parent?).

I updated the diff by setting parent_tag to sc_parent_die.Tag() instead of die.GetParent().Tag(). That fixed the issue with global constants in namespace and didn't cause other issues when running ninja check-lldb.

3281–3282

Added tests for constants in anonymous namespace. Also note that lookup for ns::var doesn't work right now (the "(anonymous namespace)" part is also expected).

[clayborg]

LGTM!

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

if (!mangled) {
     // 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 ||
+         parent_tag == DW_TAG_namespace) &&
         Language::LanguageIsCPlusPlus(GetLanguage(*die.GetCU())))
       mangled =
           GetDWARFDeclContext(die).GetQualifiedNameAsConstString().GetCString();
   }
 
   if (tag == DW_TAG_formal_parameter)
     scope = eValueTypeVariableArgument;
   else {

lldb/test/API/lang/cpp/global_variables/TestCPPGlobalVariables.py

 class GlobalVariablesCppTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     def setUp(self):
         TestBase.setUp(self)
         self.source = lldb.SBFileSpec('main.cpp')
 
-    @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24764")
     def test(self):
         self.build()
 
         (target, _, _, _) = lldbutil.run_to_source_breakpoint(self, "// Set break point at this line.", self.source)
-        
         # Check that we can access g_file_global_int by its name
         self.expect("target variable g_file_global_int", VARIABLES_DISPLAYED_CORRECTLY,
                     substrs=['42'])
         self.expect("target variable abc::g_file_global_int", VARIABLES_DISPLAYED_CORRECTLY,
                     substrs=['42'])
         self.expect("target variable xyz::g_file_global_int", VARIABLES_DISPLAYED_CORRECTLY,
                     error=True, substrs=['can\'t find global variable'])
 
+        # Check that we can access g_file_global_const_int by its name
+        self.expect("target variable g_file_global_const_int", VARIABLES_DISPLAYED_CORRECTLY,
+                    substrs=['1337'])
+        self.expect("target variable abc::g_file_global_const_int", VARIABLES_DISPLAYED_CORRECTLY,
+                    substrs=['1337'])
+        self.expect("target variable xyz::g_file_global_const_int", VARIABLES_DISPLAYED_CORRECTLY,
+                    error=True, substrs=['can\'t find global variable'])
+
+    @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24764")
+    def test_access_by_mangled_name(self):
+        self.build()
+
+        (target, _, _, _) = lldbutil.run_to_source_breakpoint(self, "// Set break point at this line.", self.source)
+
         # Check that we can access g_file_global_int by its mangled name
         addr = target.EvaluateExpression("&abc::g_file_global_int").GetValueAsUnsigned()
         self.assertNotEqual(addr, 0)
         mangled = lldb.SBAddress(addr, target).GetSymbol().GetMangledName()
         self.assertNotEqual(mangled, None)
         gv = target.FindFirstGlobalVariable(mangled)
         self.assertTrue(gv.IsValid())
         self.assertEqual(gv.GetName(), "abc::g_file_global_int")
         self.assertEqual(gv.GetValueAsUnsigned(), 42)

lldb/test/API/lang/cpp/global_variables/main.cpp

 #include <stdio.h>
 
 namespace abc {
-  int g_file_global_int = 42;
+const int g_file_global_const_int = 1337;
 }
 
 int main (int argc, char const *argv[])
 {
-    return abc::g_file_global_int; // Set break point at this line.
+  int unused = abc::g_file_global_const_int;
 }