[Lldb-commits] [PATCH] D92223: [lldb] Add support for looking up static const members

Mon Nov 30 03:21:33 PST 2020

werat added a comment.

Pavel, thanks for review!

In D92223#2422184 <https://reviews.llvm.org/D92223#2422184>, @labath wrote:

> I am not sure if this is the right way to implement this feature. Changing ManualDWARFIndex to provide this additional information is easy enough, but it means that the other index classes will never be able to support this functionality (without, essentially, reimplementing ManualDWARFIndex and scanning all debug info). 
> ...
> I think it might be better to do something at the `SymbolFileDWARF::FindGlobalVariables` level, so that it is common to all indexes. For example, this function could (in case the normal search does not provide any information) try to strip the last component of the name (`foo::bar::baz` => `foo::bar`) and then try to look up the result as a class (type). If it finds the type, it could then check it for the specific (static) variable name.

This makes sense, let me try to implement this approach. A few questions before I jump into coding. Do you suggest to call `m_index->GetTypes` from `SymbolFileDWARF::FindGlobalVariables` and inspect the DIE for static members? Or use something more "high-level", e.g. `SymbolFileDWARF::FindTypes` and inspect the returned `lldb_private::Type` object? I had a brief look at `Type` and didn't find an easy way to get to static members there. Can you give me a pointer?

> I am also worried about the increase to the manual index size, as this would mean that every occurrence of the DW_TAG_member would be placed in the index, whereas now we only place the one which has a location (which is normally just in a single compile unit).

Regarding the index size, we don't put ALL occurrences of `DW_TAG_member` in the index, only the ones with constant values. So it should be the same as if all these members had a location.

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3248
+      (parent_tag == DW_TAG_compile_unit || parent_tag == DW_TAG_partial_unit ||
+       tag == DW_TAG_member) &&
+      (parent_context_die.Tag() == DW_TAG_class_type ||
----------------
labath wrote:
> Why is this needed. I would expect this to evaluate to `true` even without this addition...
In case of `DW_TAG_member` parent_tag is `DW_TAG_class_type/DW_TAG_structure_type`, so this check doesn't pass.

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3270
     if ((parent_tag == DW_TAG_compile_unit ||
-         parent_tag == DW_TAG_partial_unit) &&
+         parent_tag == DW_TAG_partial_unit || tag == DW_TAG_member) &&
         Language::LanguageIsCPlusPlus(GetLanguage(*die.GetCU())))
----------------
labath wrote:
> Same here.
As as above, `parent_tag` for `DW_TAG_member` is `DW_TAG_class_type/DW_TAG_structure_type`

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3518
       if ((tag == DW_TAG_variable) || (tag == DW_TAG_constant) ||
+          (tag == DW_TAG_member) ||
           (tag == DW_TAG_formal_parameter && sc.function)) {
----------------
labath wrote:
> Why is this needed? I wouldn't expect a member inside a function...
I'm not sure I understand, wdym by "member inside a function"?  `ParseVariables` is called directly from `FindGlobalVariables`, so as far as I understand it this check applies for processing global variables as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92223