Page MenuHomePhabricator

[llvm-dwarfdump] Add the --show-sections-sizes option
ClosedPublic

Authored by djtodoro on Feb 7 2020, 3:08 AM.

Details

Summary

Add an option to llvm-dwarfdump to calculate the bytes within .debug* sections.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
djtodoro added inline comments.Mar 17 2020, 4:54 AM
llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test
15–16 ↗(On Diff #250140)

I would keep as is, and we can modify it if starts looking ugly. :)

llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test
1 ↗(On Diff #250140)

Sure, that makes sense!

llvm/tools/llvm-dwarfdump/SectionSizes.cpp
17

I still don't understand the +2. If all the section names are shorter than "Section", then the column will be two bytes wider than necessary. However, this +2 is not included if one or more of the section names is longer than "Section" + 2. If you want to make the column 2 wider than the names, that's fine (but why?), but you should add the +2 before returning, rather than in the initial value.

OK, I do not have a strong opinion on this, and I will agree on that. Thanks.

31

Please take a look into the output now, if it still looks it should be included, I will include the dashes for the percentage part as well.
I think the percentage is only follow-up for the size info, and I feel like not including the dashes somehow emphasize that, but again, I do not feel I have a strong opinion about that, so I am open to change that :)

llvm/tools/llvm-dwarfdump/SectionSizes.h
39

It looks better, thanks!

djtodoro updated this revision to Diff 251016.Mar 18 2020, 2:29 AM

-Use '\n' instead of "\n", since it is more efficient

I think this is starting to look good now. I found two more opportunities for improving the output... sorry about finding these piecemeal.

llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test
6 ↗(On Diff #251016)

What's the unit of SIZE? Should we print that?

24 ↗(On Diff #251016)

Should we right-align all the numbers?

56
 0
jhenderson added inline comments.Mar 19 2020, 2:49 AM
llvm/test/tools/llvm-dwarfdump/X86/section_sizes_no_debug_sections.test
7

Rather than having a special message here, I'd probably just print the Total Size and Total File Size lines as before (which will be 0 and the object file size).

17–22

Remove the extra spaces between the keys and values.

llvm/tools/llvm-dwarfdump/SectionSizes.cpp
16

No need for the namespace here, since you are using namespace llvm above.

32

Typo: SectionSizeTitle

llvm/tools/llvm-dwarfdump/SectionSizes.h
33–38

It looks like these two functions are no longer defined after you renamed them in the source file? If they aren't used outside the source file, just make them static functions in the .cpp.

Same goes for everything in here - only include in the header what needs to be part of the public interface.

Forgot to mention: given this is intended to be common across other file formats, and there is a need to test the isDebugSection functions, I'd recommend adding tests for COFF and/or Mach-O as well as the ELF one. They don't need to be comprehensive necessarily.

djtodoro marked 4 inline comments as done.Mar 20 2020, 2:28 AM

@aprantl @jhenderson Thanks a lot for the reviews! Now we have tests for ELF(section_sizes.test), COFF(coff-x86_64.yaml) and MACH-O(macho_archive_section_sizes.test).

llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test
6 ↗(On Diff #251016)

bytes, and we can hard code them now, but when we add support for different units, that could be dynamically printed.

24 ↗(On Diff #251016)

Good idea, thanks!

llvm/test/tools/llvm-dwarfdump/X86/section_sizes_no_debug_sections.test
7

Sounds good, thanks!

llvm/tools/llvm-dwarfdump/SectionSizes.cpp
32

Oh, I uploaded a wrong diff :(

djtodoro updated this revision to Diff 251575.Mar 20 2020, 2:28 AM

-addressing comments

aprantl accepted this revision.Mar 20 2020, 4:25 PM

Looks like a great start! (From my side)

llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test
9 ↗(On Diff #251575)

long-term for the human readable output we probably want a function that takes byte value and prints it the way ls -l -h formats number, if we don't already have one in LLVM.

This revision is now accepted and ready to land.Mar 20 2020, 4:25 PM
jhenderson added inline comments.Mar 23 2020, 2:26 AM
llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test
1 ↗(On Diff #251575)

You should avoid using a precanned binary if possible, especially one from a completely different tool's testing. yaml2obj has Mach-O support, so I'd recommend using that (although I don't know how to create fat archives).

The new file should have section names in the ELF style too, to show that they are not included.

6 ↗(On Diff #251575)

I might suggest (bytes) explicitly, for clarity, but I don't mind, if there's some pre-existing style this is following elsewhere.

llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test
46 ↗(On Diff #251575)

You might want to add a section with a Mach-O naming style to show that it is NOT included.

llvm/test/tools/llvm-dwarfdump/coff-x86_64.yaml
2 ↗(On Diff #251575)

Rather than folding it in here, I'd keep this test in a separate test file, or even just add it to section_sizes.test (you might need to copy the YAML across too of course).

Same comment then goes as the other tests re. section names.

llvm/tools/llvm-dwarfdump/SectionSizes.cpp
30

To avoid rotting once support for other units is added, it might make more sense to say "the size of the column title".

81–82

You still don't need the namespace llvm since you have a using namespace llvm at the top of this file.

djtodoro marked 7 inline comments as done.Mar 23 2020, 7:49 AM

Thanks!

llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test
1 ↗(On Diff #251575)

You should avoid using a precanned binary if possible, especially one from a completely different tool's testing. yaml2obj has Mach-O support, so I'd recommend using that (although I don't know how to create fat archives).

There are cases, even in llvm-dwarfdump, using the files from Input/, but if you think so, I can delete it.

The new file should have section names in the ELF style too, to show that they are not included.

I've added a new test case for this.

6 ↗(On Diff #251575)

I see it is being used as is. Also, I put an explanation in the 'help' for the option.

9 ↗(On Diff #251575)

I agree.

llvm/tools/llvm-dwarfdump/SectionSizes.cpp
81–82

This is needed to avoid an ambiguous call to the calculateSectionSizes().

djtodoro updated this revision to Diff 252041.Mar 23 2020, 7:54 AM

-adding new tests

jhenderson added inline comments.Mar 23 2020, 9:14 AM
llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test
1 ↗(On Diff #251575)

Using assembly etc from the Inputs folder local to the current tool is fine, but where possible, we try to avoid pre-canned binaries these days, as they are opaque and hard to maintain. Referencing anotehr tool's Inputs folder is particularly bad as it creates an unexpected dependency between the two sets of tests.

llvm/test/tools/llvm-dwarfdump/X86/section_sizes_coff.test
2

Add a comment like the other test.

27

If I'm not mistaken, these don't need to be valid data. Does COFF yaml2obj provide the "Size:" tag as an option for setting the section size? If not, I'd just use zeroes to write the data. Same goes below.

35

"This is the debug section following the Mach-O naming style, so this is not included in the report." -> "This is a debug section following the Mach-O naming style, and is used to show that it such sections are not included in the report."

llvm/test/tools/llvm-dwarfdump/X86/section_sizes_elf.test
2

Add "for ELF objects" to the end here.

50

Same comment as above.

llvm/test/tools/llvm-dwarfdump/X86/section_sizes_mach_o.test
1 ↗(On Diff #252041)

Rename this test to section_sizes_macho.test for consistency with typical naming styles elsewhere.

Please add a top-level comment too.

Any particular reason you're creating an intermediate object here and not in the COFF case?

37 ↗(On Diff #252041)

Same basic comment as in other tests.

llvm/tools/llvm-dwarfdump/SectionSizes.cpp
30

Sorry, I guess this wasn't quite clear enough. I meant to replace the "size of "SIZE (b)"" with "size of the column title" here. So the full sentence is "The minimum column width should be the size of the column title."

81–82

What ambiguous call? There is only one function called calculateSectionSizes() defined here...

djtodoro marked 2 inline comments as done.Mar 24 2020, 5:31 AM
djtodoro added inline comments.
llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test
1 ↗(On Diff #251575)

OK, I'll remove the test.

llvm/tools/llvm-dwarfdump/SectionSizes.cpp
81–82

Without even llvm:: here I get:

SectionSizes.cpp:109:3: error: call to 'calculateSectionSizes' is ambiguous

maybe I am oversighting something very obvious here?

djtodoro updated this revision to Diff 252284.Mar 24 2020, 5:33 AM

-refactoring the testcases (addressing comments)

Code all looks fine now, just a few minor test suggestions.

What happened to "macho_archive_section_sizes.test"? I thought that was asked for for testing fat archives? On that note, it might be worth a test for llvm-ar produced archives too.

llvm/test/tools/llvm-dwarfdump/X86/section_sizes_coff.test
27

Perhaps worth having one of these with a non-zero size?

37

Split this over two lines. Also, there was a typo in my original suggestion: "show that it such" -> "show that such"

llvm/test/tools/llvm-dwarfdump/X86/section_sizes_elf.test
50

Ditto.

llvm/test/tools/llvm-dwarfdump/X86/section_sizes_macho.test
40

Same comment as elsewhere.

llvm/tools/llvm-dwarfdump/SectionSizes.cpp
81–82

Never mind, I'm just being a bit dumb, I think.

djtodoro marked 2 inline comments as done.Mar 26 2020, 8:00 AM

What happened to "macho_archive_section_sizes.test"? I thought that was asked for for testing fat archives? On that note, it might be worth a test for llvm-ar produced archives too.

I'll add one. Thanks.

llvm/test/tools/llvm-dwarfdump/X86/section_sizes_coff.test
27

Makes sense to me.

35

Oh, my bad, I should have checked it.

djtodoro updated this revision to Diff 252850.Mar 26 2020, 8:03 AM

-refactoring the tests
-adding a test for an archive

Hi @aprantl,

Do you think a regular llvm-ar archive test is sufficient for this testing, or do you think a mach-o fat archive is important to test independently? I don't see a massive need for the latter personally.

llvm/test/tools/llvm-dwarfdump/X86/section_sizes_archive.test
14

Please include the actual file path here and below. You may need to use FileCheck's -D to achieve this, e.g. FileCheck %s -DFILE1=%t.1.o -DFILE2=%t.2.o --match-full-lines --strict-whitespace.

24–25

You should probably make this a continuous series of CHECK-NEXT/CHECK-EMPTY, to show what the formatting between blocks looks like.

29–30

Change one of these two sections to use the same name as one from the first object, to show that the sizes are independent.

djtodoro updated this revision to Diff 253102.Mar 27 2020, 6:48 AM

-refactor the archive testcase

djtodoro added a comment.EditedMar 27 2020, 6:58 AM

I think that @aprantl wanted a test for Mach-O and archives, back then. I've added a test for mach-o and archive, in one shot (by using pre-existing fat mach-o archive from dsymutil). Then, I've added different tests for individual targets (elf, mach-o and coff) in order to confirm we use the right isDebugSection() and by not including a debug section from different name styling (we weren't able to check that with existing mach-o test, since we had the pre-existing fat mach-o archive from dsymutil test). Furthermore, we decided not to use pre-existing Inputs/ from different tools, so I've added the llvm/test/tools/llvm-dwarfdump/X86/section_sizes_archive.test, by testing archives. I don't think we would test something new by adding another fat archive test.

I think that @aprantl wanted a test for Mach-O and archives, back then. I've added a test for mach-o and archive, in one shot (by using pre-existing fat mach-o archive from dsymutil).

What makes the dsymutil test special is that you can have fat (=multi-architecture) Mach-O binaries inside a static archive and it is important to test that this case is handled correctly, because you have two layers of multi-file enumeration going on. Since we already have it checked in as a binary I thought reusing it would be preferable over checking in yet another opaque binary. If we can generate one one the fly, that's fine, too.

I think that @aprantl wanted a test for Mach-O and archives, back then. I've added a test for mach-o and archive, in one shot (by using pre-existing fat mach-o archive from dsymutil).

What makes the dsymutil test special is that you can have fat (=multi-architecture) Mach-O binaries inside a static archive and it is important to test that this case is handled correctly, because you have two layers of multi-file enumeration going on. Since we already have it checked in as a binary I thought reusing it would be preferable over checking in yet another opaque binary. If we can generate one one the fly, that's fine, too.

I'm not at all familiar with Mach-O, so how do fat archives get generated (i.e. with what tool?)? Is there an LLVM equivalent we can use here?

I'm not at all familiar with Mach-O, so how do fat archives get generated (i.e. with what tool?)? Is there an LLVM equivalent we can use here?

There is no such thing as a fat archive, just regular archives that contain fat object files. The tool to create fat binaries is called lipo and there seems to be an llvm variant of it.

I'm not at all familiar with Mach-O, so how do fat archives get generated (i.e. with what tool?)? Is there an LLVM equivalent we can use here?

There is no such thing as a fat archive, just regular archives that contain fat object files. The tool to create fat binaries is called lipo and there seems to be an llvm variant of it.

Thanks! I'm sure I heard them referred to fat archives before...! @djtodoro, could you have a look at using llvm-lipo to create a simple test case for fat binaries in archives?

I'm not at all familiar with Mach-O, so how do fat archives get generated (i.e. with what tool?)? Is there an LLVM equivalent we can use here?

There is no such thing as a fat archive, just regular archives that contain fat object files. The tool to create fat binaries is called lipo and there seems to be an llvm variant of it.

Thanks! I'm sure I heard them referred to fat archives before...! @djtodoro, could you have a look at using llvm-lipo to create a simple test case for fat binaries in archives?

Sure.

djtodoro updated this revision to Diff 253839.Mar 31 2020, 4:45 AM

-adding a test for fat binaries

jhenderson accepted this revision.Mar 31 2020, 6:20 AM

LGTM, with one last request.

llvm/test/tools/llvm-dwarfdump/X86/section_sizes_coff.test
8

Some of the tests are missing this first line. Please fix them to all check it.

djtodoro updated this revision to Diff 253872.Mar 31 2020, 6:59 AM

-addressing comments

jhenderson added inline comments.Mar 31 2020, 7:12 AM
llvm/test/tools/llvm-dwarfdump/X86/section_sizes_archive.test
15

CHECK-NEXT here and in all tests you just updated.

djtodoro updated this revision to Diff 253878.Mar 31 2020, 7:18 AM

-refactor the tests

jhenderson accepted this revision.Mar 31 2020, 7:49 AM

@jhenderson @aprantl Thanks for the review.

Thanks for seeing it through!

This revision was automatically updated to reflect the committed changes.

I'm not at all familiar with Mach-O, so how do fat archives get generated (i.e. with what tool?)? Is there an LLVM equivalent we can use here?

There is no such thing as a fat archive, just regular archives that contain fat object files. The tool to create fat binaries is called lipo and there seems to be an llvm variant of it.

Turns out I was wrong about that. You can also have fat .a files (created by lipo).