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

[Greg Clayton via Phabricator via lldb-commits]

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


================
Comment at: lldb/test/API/lang/cpp/global_variables/main.cpp:5
+int g_file_global_int = 42;
+const int g_file_global_const_int = 1337;
 }
----------------
tonkosi wrote:
> clayborg wrote:
> > 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
> 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`.
ok, thanks for trying


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112147