[Lldb-commits] [PATCH] D112147: [lldb] Fix lookup for global constants in namespaces

[Tonko Sabolčec via Phabricator via lldb-commits]

tonkosi added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3281-3282
     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())))
----------------
shafik wrote:
> clayborg wrote:
> > tonkosi wrote:
> > > clayborg wrote:
> > > > 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.
> > > I tried to call `GetDWARFDeclContext(die)` in a general case, but it seems that `GetQualifiedName` will append `::` ([source](https://github.com/llvm/llvm-project/blob/5015f250894d3d97917d858850fae7960923a4ae/lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.cpp#L22)) 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?
> > 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.
> You should document the anonymous namespace case in a test.
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`.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3281-3282
     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())))
----------------
tonkosi wrote:
> shafik wrote:
> > clayborg wrote:
> > > tonkosi wrote:
> > > > clayborg wrote:
> > > > > 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.
> > > > I tried to call `GetDWARFDeclContext(die)` in a general case, but it seems that `GetQualifiedName` will append `::` ([source](https://github.com/llvm/llvm-project/blob/5015f250894d3d97917d858850fae7960923a4ae/lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.cpp#L22)) 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?
> > > 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.
> > You should document the anonymous namespace case in a test.
> 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`.
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112147/new/

https://reviews.llvm.org/D112147