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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
