Page MenuHomePhabricator

[lldb] Add support for looking up static const members
Needs ReviewPublic

Authored by werat on Nov 27 2020, 5:53 AM.

Details

Summary

Static const/constexpr members might have only DW_TAG_member entry with a
DW_AT_const_value attribute. Since there is no corresponding DW_TAG_variable
entry, these values are currently not indexed in the DWARF index and are not
available via SBTarget::FindGlobalVariables() API.

Diff Detail

Event Timeline

werat created this revision.Nov 27 2020, 5:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2020, 5:53 AM
werat requested review of this revision.Nov 27 2020, 5:53 AM
teemperor added a subscriber: teemperor.

Not sure who's the right person to review the SymbolFileDWARF.cpp/ManualDWARFIndex.cpp, but Pavel/Jan touched that not too long ago.

(D81471 might also be an interested patch for you. That one is just waiting on me to add some of the requested tests).

lldb/test/API/python_api/target/globals/TestTargetGlobals.py
6

Unused import

9

Also unused?

46

The code between here and self.build() can be simplified to:

 self.build()
target, _, _, _ lldbutil.run_to_source_breakpoint(self, "// Set a break at entry to main.", lldb.SBFileSpec("main.cpp"))

And then the setUp can also be removed.

FWIW, I believe we don't need to launch any process for this test, so this might be enough to test this (while being a much faster test):

self.build()
target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
werat added reviewers: jarin, aprantl, shafik.EditedNov 27 2020, 7:27 AM

Hi, please, take a look at this patch.

It seems Pavel already did some work for class-level static const variables in https://reviews.llvm.org/D86615, although that didn't work the case I've encountered.

In my example (which is also reflected in the test I've added), class-level static constexpr variable is presented in DWARF only via DW_TAG_member with DW_AT_const_value. The code on the lookup path looks only at DW_TAG_variable entries, which are not present.

werat updated this revision to Diff 308052.EditedNov 27 2020, 7:32 AM

Simplified the test according to teemperor's comments.

werat marked 3 inline comments as done.Nov 27 2020, 7:33 AM
werat updated this revision to Diff 308053.Nov 27 2020, 7:36 AM

Remove unused field

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.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3183

typo

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

3248

Why is this needed. I would expect this to evaluate to true even without this addition...

3270

Same here.

3518

Why is this needed? I wouldn't expect a member inside a function...

lldb/test/API/python_api/target/globals/TestTargetGlobals.py
15–17

delete

werat added a comment.Nov 30 2020, 3:21 AM

Pavel, thanks for review!

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.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3248

In case of DW_TAG_member parent_tag is DW_TAG_class_type/DW_TAG_structure_type, so this check doesn't pass.

3270

As as above, parent_tag for DW_TAG_member is DW_TAG_class_type/DW_TAG_structure_type

3518

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.

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

I wanted to post here rather my agreement with the patch. This looks to me as giving up on the high performance index benefits. (But maybe what you say is the right sweet spot compromise.)

I rather find missing a feature to cross-check ManualDWARFIndex vs. on-disk indexes (lld/LTO produced .debug_names, gdb-add-index -dwarf-5 produced .debug_names and how Apple produces their index). Fedora is probably going to have two indexes (.gdb_index and .debug_names) for each LLVM-built binary as the format of GDB .debug_names is currently incompatible with LLVM/LLDB .debug_names. I also expect current ManualDWARFIndex is producing different index than lld.

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

With -flto (even with -O0) there is only a single definition of each class.

FYI it would not work with GNU C++ 4.4 (2008, RHEL-5) as it used DW_TAG_variable instead of DW_TAG_member. But that is probably not a problem, Red Hat's last supported RHEL for new toolchains is since RHEL-6 and for LLVM even just since RHEL-7:

<0><b>: Abbrev Number: 1 (DW_TAG_compile_unit)
    DW_AT_producer    : GNU C++ 4.1.2 20080704 (Red Hat 4.1.2-55)
    DW_AT_language    : 4      (C++)
    ...
<1><70>: Abbrev Number: 2 (DW_TAG_class_type)
    DW_AT_name        : C
    ...
<2><7a>: Abbrev Number: 3 (DW_TAG_variable)
    DW_AT_name        : i
    DW_AT_decl_file   : 1
    DW_AT_decl_line   : 2
    DW_AT_MIPS_linkage_name: _ZN1C1iE
    DW_AT_type        : <90>
    DW_AT_external    : 1
    DW_AT_accessibility: 3     (private)
    DW_AT_declaration : 1
shafik added inline comments.Nov 30 2020, 2:04 PM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3181

This is a nitpick but if we move the comment above the if we can remove the braces.

This patch may be superseded by D92643, not sure.

werat added a comment.Feb 6 2021, 1:45 AM

This patch may be superseded by D92643, not sure.

Yeah, D92643 is an alternative way to implement this, according to @labath comments. Sorry I didn't make it clear.

Based on https://reviews.llvm.org/D92643#inline-925368 where the DIE traversal cannot performance-reasonably implement target variable -r I am returning here whether the .debug_names index could not be rather extended/improved. LLDB would fall back to ManualDWARFIndex if the on-disk index version is lower than what LLDB knows. For example GDB .gdb_index is now at version 8, GDB has also a use-deprecated-index-sections setting.

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

Only occurences which define (not declare) a static const member would be stored. I have measured it using https://people.redhat.com/jkratoch/lldb-staticconst.patch tested on a sample file bin/clang-13 being built -O0 -g -fno-lto using:

time cmake ../llvm-monorepo/llvm/ -DCMAKE_BUILD_TYPE=Debug -DLLVM_USE_LINKER=lld -DLLVM_ENABLE_PROJECTS="lldb;clang;lld" -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_OPTIMIZED_TABLEGEN=ON -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_USE_SPLIT_DWARF=OFF -DCMAKE_CXX_FLAGS="-gdwarf-5 -gpubnames" -DCMAKE_C_FLAGS="-gdwarf-5 -gpubnames" -GNinja;ninja ...;llvm-objcopy -R .debug_names bin/clang-13

And it increases the index size by 1.4% (by number of DW_IDX_die_offset entries, 14964676+210528). I find such increase worth the static const members effective access. The question is whether bin/clang-13 is a good sample binary.
That sure assumes an upstream DWARF discussion+update of the .debug_names format (and then there is the Apple index).

The static const members could be (based on a DWARF discussion) stored also only for one CU but I do not think it is needed given the size increase is not significant.

This comment was removed by jankratochvil.

The considered .debug_names index update would need to make .debug_names working in the first place - as it does not work now: D99850
So maybe the manual DIE traversal without updating the index and without the regex capability is OK but I do not want to give the agreement myself.