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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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

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.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3183
+    // Allows only members with DW_AT_const_value attribute, i.e. static const
+    // or static constexr.
+    return nullptr;
----------------
typo

(also "static constexpr" implies "static const" so maybe there's no need to explicitly mention the latter...)


================
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 ||
----------------
Why is this needed. I would expect this to evaluate to `true` even without this addition...


================
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())))
----------------
Same here.


================
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)) {
----------------
Why is this needed? I wouldn't expect a member inside a function...


================
Comment at: lldb/test/API/python_api/target/globals/TestTargetGlobals.py:15-17
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
----------------
delete


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92223