Page MenuHomePhabricator

[lldb] Fix lookup for global constants in namespaces
ClosedPublic

Authored by tonkosi on Oct 20 2021, 9:02 AM.

Details

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.

Diff Detail

Event Timeline

tonkosi requested review of this revision.Oct 20 2021, 9:02 AM
tonkosi created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2021, 9:02 AM
werat accepted this revision.Oct 22 2021, 5:42 AM

This looks reasonable to me.

@teemperor any thoughts?

This revision is now accepted and ready to land.Oct 22 2021, 5:42 AM

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

clayborg requested changes to this revision.Oct 22 2021, 10:15 AM
clayborg added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3297–3298

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

This revision now requires changes to proceed.Oct 22 2021, 10:15 AM
tonkosi added inline comments.Tue, Nov 9, 11:37 AM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3297–3298

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 added inline comments.Tue, Nov 9, 3:14 PM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3297–3298

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 added a subscriber: shafik.Tue, Nov 9, 4:38 PM
shafik added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3297–3298

You should document the anonymous namespace case in a test.

tonkosi updated this revision to Diff 388958.Mon, Nov 22, 9:56 AM
  • Use scope context parent DIE instead of the direct parent
  • Add tests for constants in anonymous namespace
tonkosi added inline comments.Mon, Nov 22, 10:03 AM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3297–3298

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.

3297–3298

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

This revision is now accepted and ready to land.Mon, Nov 22, 10:54 AM
This revision was automatically updated to reflect the committed changes.