Page MenuHomePhabricator

[lldb][DWARF] Added support for new forms in DWARFv5 macro.
Needs ReviewPublic

Authored by SouraVX on Jan 13 2020, 3:47 AM.

Details

Summary

This patch adds support for DWARFv5 new forms: DW_MACRO_define_strx, DW_MACRO_undef_strx.

Diff Detail

Event Timeline

SouraVX created this revision.Jan 13 2020, 3:47 AM
SouraVX marked an inline comment as done.Jan 13 2020, 3:49 AM
SouraVX added a subscriber: dblaikie.
SouraVX added inline comments.
lldb/test/Shell/Commands/Inputs/dwarf5-macro.s
2

This file is generated via clang/llvm with macro section support. -- will upstream that patch soon.

Unit tests: pass. 61782 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

clayborg added inline comments.Jan 13 2020, 10:15 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.cpp
58–59

We could also remove "offset_is_64_bit" if we fix DataExtractor to handle it as suggested below where "offset_is_64_bit" is used.

130–133

I have though about adding support to DataExtractor for extracting offsets. Anyone that initializes a DataExtractor would need to set the offset size once, and then everyone else could just call:

new_offset = debug_macro_data.GetOffset(offset);

We would need to add a field to DataExtractor:

Optional<uint32_t> m_offset_size; ///< The address size to use when extracting offsets. Defaults to m_addr_size if this has no value

It would default to the m_addr_size if m_offset_size isn't set. The "m_context.getOrLoadXXXX()" calls would then set the offset size correctly for the current DWARF when it creates the DWARFDataExtractor.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h
55–58

To future proof this a bit should we pass in the "lldb_private::DWARFContext" instead of the first three arguments?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
1055–1056

Move this into DWARFDebugMacroEntry::ReadMacroEntries() so that we don't have to pass "header" as an argument?

shafik added a subscriber: shafik.
shafik added inline comments.
lldb/test/Shell/Commands/dwarf5-macro.test
2

Is there a reason why we would only want to test this feature on x86?

aprantl added inline comments.Jan 13 2020, 1:07 PM
lldb/test/Shell/Commands/dwarf5-macro.test
2

Presumably because the input is X86 assembly. That said, would it be possible to test the feature without constructing a process, so we *can* test it everywhere?

SouraVX updated this revision to Diff 237909.Jan 14 2020, 3:25 AM

Thank you everyone, for taking out time and reviewing this.
Addressed @clayborg review comments.

I have a high-level question/comment. Are you planning to implement debug_macro reading in llvm-dwarfdump? Even if you aren't, I am expecting that you will be asked to do that as a part of testing for your debug_macro generation patch... And in that case, it would be much better to write a debug_macro parser in first, and reuse that in lldb, similar to how its done for rnglists, loclists, and other "minor" sections. The slightly tricky part there is that you need to make sure to not intertwine it too much with other llvm dwarf classes, as lldb does not use all of llvm dwarf (yet). However, given that the debug_macro section is pretty independent, that shouldn't be too much of a problem?

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.cpp
70

Surely, this should be set according to the actual str_offsets_base value, no?

SouraVX marked 4 inline comments as done.Jan 14 2020, 3:33 AM
SouraVX added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.cpp
58–59

Agreed, however I think this enhancement has to be done separately.

lldb/test/Shell/Commands/dwarf5-macro.test
2

I tried, other ways also. Seems like this is bare minimum, We need to run the program to be able to expand macros. analogous behavior with GDB also.
If you've some better way in mind to test this. or where to put this. Please share.

I have a high-level question/comment. Are you planning to implement debug_macro reading in llvm-dwarfdump? Even if you aren't, I am expecting that you will be asked to do that as a part of testing for your debug_macro generation patch...

Hi Pavel, I've mostly completed debug_macro generation[clang/llvm] and dumping it using llvm-dwarfdump. I'll soon file a review for that also. Maybe we can address this concern while reviewing that. Till that, I think it would be nice to have minimum support for macros.

And in that case, it would be much better to write a debug_macro parser in first, and reuse that in lldb, similar to how its done for rnglists, loclists, and other "minor" sections. The slightly tricky part there is that you need to make sure to not intertwine it too much with other llvm dwarf classes, as lldb does not use all of llvm dwarf (yet). However, given that the debug_macro section is pretty independent, that shouldn't be too much of a problem?

I have a high-level question/comment. Are you planning to implement debug_macro reading in llvm-dwarfdump? Even if you aren't, I am expecting that you will be asked to do that as a part of testing for your debug_macro generation patch...

Hi Pavel, I've mostly completed debug_macro generation[clang/llvm] and dumping it using llvm-dwarfdump. I'll soon file a review for that also. Maybe we can address this concern while reviewing that. Till that, I think it would be nice to have minimum support for macros.

I don't think this issue is particularly urgent -- a consumer is not very useful if you don't have a producer which can produce that data. And reusing the parser in llvm would allow us to largely drop the testing part of this discussion -- if the nitty-gritty encoding details are tested in llvm, then here we just need to make sure the integration works, and we already mostly have a test for that (TestMacros.py).

lldb/test/Shell/Commands/dwarf5-macro.test
2

It wouldn't be unreasonable to add some command to query active macro definitions at a given point in the program (similar to gdb's info macros command). In lldb, the best place to plug this into might be the "image lookup" command -- right now "image lookup --verbose --file a.cpp --line 10" will give you the function containing that line as well as all variables which are active at that point. It seems natural to also list the active macros too.

However, that would most likely require adding a new "macros" field to the SymbolContext class, which is a concept used throughout lldb, and so doing it may be fairly tricky and it may be better to create a separate command for this at first ("image dump macros"?)...

(also note that in the current form, this test will not only require x86, but also a linux (or at least elf) system)