Page MenuHomePhabricator

[LLDB] Support GNU-style compressed debug sections (.zdebug)
ClosedPublic

Authored by alur on Apr 13 2018, 10:14 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

alur created this revision.Apr 13 2018, 10:14 AM
davide requested changes to this revision.Apr 13 2018, 10:19 AM
davide added subscribers: jasonmolenda, davide.

This requires an unittest (or an lldb-test test). Some comments inline.
I think @clayborg or @jasonmolenda are in a better position to review the structure, I can take a look at the ELF specific details.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1890–1891 ↗(On Diff #142431)

this whole cascade is really not ideal. Maybe we should factor the checks out or move this to be a switch?

2113–2115 ↗(On Diff #142431)

Please remove references to GOOGLE3.

This revision now requires changes to proceed.Apr 13 2018, 10:19 AM
alur updated this revision to Diff 142435.Apr 13 2018, 10:21 AM
alur marked 2 inline comments as done.Apr 13 2018, 10:23 AM
alur marked an inline comment as not done.

Which compilers / platforms generate / support this? Is this an ELF-only feature?

clayborg added inline comments.Apr 13 2018, 10:36 AM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2937–2938 ↗(On Diff #142435)

Might be worth making these llvm::StringRef, then see comments below..

2948 ↗(On Diff #142435)

Make this a StringRef:

llvm::StringRef section_name = section->GetName().GetStringRef();
2954–2955 ↗(On Diff #142435)

use StringRef::startswith:

if (section_name.startswith(debug_prefix) || section_name.startswith(zdebug_prefix))
3451 ↗(On Diff #142435)

use "section_name" here instead of "section->GetName().GetStringRef()" since we switched it to a StringRef

3472 ↗(On Diff #142435)

Ditto

If this is for current and future debugging, it would be nice to change the tool to just use the normal .debug prefixes and just specify SHF_COMPRESSED???

alur added a subscriber: dblaikie.Apr 13 2018, 2:02 PM

Which compilers / platforms generate / support this? Is this an ELF-only feature?

Clang/LLVM do (-Wa,--compress-debug-sections). Yeah, it's ELF only so far as I know. There's a couple of variations (-compress-debug-sections=zlib or zlib-gnu) the ".zdebug_foo" is zlib-gnu style, but a more modern variant (zlib) uses a section attribute SHF_COMPRESSED instead of the name mangling.

If this is for current and future debugging, it would be nice to change the tool to just use the normal .debug prefixes and just specify SHF_COMPRESSED???

At least for now, zlib-gnu style is used at Google - I don't actually know what it'd cost to switch to the more modern zlib (using SHF_COMPRESSED) style. No doubt there are a variety of tools that may not have been updated/improved to support the new format - hard to say.

Thanks for clarifying. You'll also need to add a testcase. If clang supports this then I don't have a problem with supporting this in LLDB and adding a testcase should be easy.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1893 ↗(On Diff #142435)

Could this entire device be replaced by a llvm::StringSwitch or something else more elegant?

We should test both ways: using normal DWARF section names with SHF_COMPRESSED, and with ".zdebug" prefixed section names.

alur updated this revision to Diff 142478.Apr 13 2018, 4:43 PM
alur marked 7 inline comments as done.

I refactored the code to address the comments on it. I'll add those tests once I get the test runner to work.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1893 ↗(On Diff #142435)

I moved it out into a separate method with a StringSwitch

3451 ↗(On Diff #142435)

This is a separate method, which doesn't have a section_name variable.

3472 ↗(On Diff #142435)

Same here.

davide added inline comments.Apr 13 2018, 4:48 PM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1773–1781 ↗(On Diff #142478)

Thanks! This was exactly what I had in mind. Do you mind to split the NFC changes into a separate patch (that can be committed without review)? This will make this patch much easier to review.

Thank you for the patch. For testing I'd recommend taking a look at r320813 https://reviews.llvm.org/D40616, which implemented the SHF_COMPRESSED part of the compressed section support. It looks like we should add a new field to the "lldb-test module-sections" output, so that we can test the section type computation code.

alur updated this revision to Diff 142862.Apr 17 2018, 4:41 PM
alur updated this revision to Diff 142869.Apr 17 2018, 4:50 PM
alur marked an inline comment as done.Apr 17 2018, 5:53 PM
alur added inline comments.
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1773–1781 ↗(On Diff #142478)

Sure, attached it here.

alur updated this revision to Diff 142875.Apr 17 2018, 6:06 PM
alur marked an inline comment as done.

Changed the diff to be against the NFCs.

clayborg accepted this revision.Apr 18 2018, 9:07 AM

Very nice!

davide requested changes to this revision.Apr 18 2018, 9:45 AM

Some minor comments, almost ready to go. Do you have commit access or you need somebody to push this on your behalf?

packages/Python/lldbsuite/test/linux/compressed-debug-info/TestCompressedDebugInfo.py
22–40 ↗(On Diff #142875)

This test is too much boilerplate. We moved to a new, more concise style. You might consider using

(target, process, thread, main_breakpoint) = lldbutil.run_to_source_breakpoint(self, 
        "break here", src_file_spec, exe_name = exe)

instead.

47–64 ↗(On Diff #142875)

ditto.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1778–1781 ↗(On Diff #142875)

Can you add a comment to explain why are you dropping the first two character here?

This revision now requires changes to proceed.Apr 18 2018, 9:45 AM
alur updated this revision to Diff 142962.Apr 18 2018, 10:39 AM
alur marked 3 inline comments as done.Apr 18 2018, 10:42 AM

Thank you, I do need someone to push this on my behalf.

alur added a comment.Apr 24 2018, 1:38 PM

Friendly ping, is there anything else I need to do for this to get submitted?

Hi Erik, the review is still marked as requiring changes. Once that is
sorted out I'd be happy to submit this on your behalf (what is the base SVN
revision for the latest patch?)

Davide Italiano, is all the CR feedback addressed in the latest revision?

This is fine, I'll commit it for you today.

alur added a comment.Apr 27 2018, 4:16 PM

Thank you David.

This is still based on the latest revision of the file (+ the non functional change patch).

alur updated this revision to Diff 144834.EditedMay 1 2018, 10:29 PM
alur edited the summary of this revision. (Show Details)

Since there was some changes to the file during the last couple of days I've updated the patches to be against HEAD again.

The new NFC patch:

alur edited the summary of this revision. (Show Details)May 1 2018, 10:30 PM
alur updated this revision to Diff 144889.May 2 2018, 8:04 AM
alur updated this revision to Diff 145939.May 9 2018, 9:13 AM
alur retitled this revision from [LLDB] Support compressed debug info sections (.zdebug*) to [LLDB] Support GNU-style compressed debug sections (.zdebug).

Updated the diff to be against head, again.

alur added a comment.May 9 2018, 9:14 AM

Friendly ping on this again. I re-merged the patch against head.

This revision was not accepted when it landed; it landed in state Needs Review.May 11 2018, 5:33 PM
This revision was automatically updated to reflect the committed changes.