This is an archive of the discontinued LLVM Phabricator instance.

[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
jhenderson added inline comments.Mar 5 2020, 2:26 AM
llvm/tools/llvm-dwarfdump/SectionSizes.cpp
34

WithColor.h has recently gained defaultWarningHandler which allows printing of errors nicely, without the extra effort here.

Also, there is a createFileError method that takes an Error. Consequently, you can write this function simply as:

WithColor::defaultWarningHandler(createFileError(Filename, std::move(Err));

You could even consider doing away with this function then, by doing this inline, but it's up to you on that one. I'm happy either way.

76

Here and throughout your code, use const Twine &. From the Twine.h comments:

"Twines should only be used accepted as const references in arguments,"

98

collectSecsSizesForObjectFile -> collectObjectSectionSizes

106

Is the file format really important? I personally don't think it gives us anything.

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
287

This seems unnecessary. The only difference is removing the DWARFContext argument, right? I'd just pass in the DWARFContext and not use it to keep the interface identical.

djtodoro updated this revision to Diff 248658.Mar 6 2020, 1:25 AM

-Addressing comments

djtodoro marked 4 inline comments as done.Mar 6 2020, 1:30 AM

@jhenderson Thanks for the review!

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

I am getting the YAML error when trying to add duplicated section here:

repeated section/fill name: ... at YAML section/fill number ...

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

Addressed with D75727, since we have more functions using it.

106

Hm... OK. I see the llvm-dwarfdump prints the file format when outputs the --stats, but we can avoid that here.

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
287

Agree, it seems reasonable.

jhenderson added inline comments.Mar 6 2020, 1:54 AM
llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test
1 ↗(On Diff #243838)

There's a special syntax for creating unique sections with the same name. See case 1 in https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/yaml2obj/ELF/duplicate-section-names.yaml.

16 ↗(On Diff #248118)

Actually, on second thoughts "Total Size" (without the colon) is probably better. I think it reads cleaner.

I'd also space it the same as the section columns above, so that it all lines up.

6 ↗(On Diff #248658)

Tip: Add spaces between the '#' and 'CHECK' so that the colons line up:

#      CHECK:file: {{.*}}
# CHECK-NEXT:----------------------------------------------------
10 ↗(On Diff #248658)

I think the section name column needs to be dynamically sized. The length of a section name isn't really limited, and indeed, there are some quite long DWARF section names out there already (e.g. .debug_cu_index or .debug_info.dwo), which might cause the formatting here to go to pot. Please add a section with a long name (e.g. change .debug_foo to .debug_arbitrary_long_name) to illustrate.

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

const Twine &
DICtx -> /*DICtx*/ and then you don't need the comment or void cast below.

106

If a use-case arises for it later, we can add it then.

djtodoro marked 3 inline comments as done.Mar 6 2020, 2:24 AM
djtodoro added inline comments.
llvm/test/tools/llvm-dwarfdump/X86/section_sizes.test
1 ↗(On Diff #243838)

Thanks!

10 ↗(On Diff #248658)

It makes sense.

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

const Twine &

It is addressed along with the D75727.

djtodoro updated this revision to Diff 248691.Mar 6 2020, 4:03 AM

-Addressing comments

Sorry for taking so long, I have three more questions:

What does the output look like for fat Mach-O binaries? There should be some in the dsymutil test inputs, if you have trouble creating one.
What does the output look like for a static .a archive?

I'm wondering if we should include the *data* collected here in the JSON output of --statistics if we don't already.

SouraVX added inline comments.Mar 6 2020, 10:07 AM
llvm/tools/llvm-dwarfdump/SectionSizes.cpp
80

nit: Since these are one liners. Can remove these braces from both if-else block?

jhenderson added inline comments.Mar 9 2020, 2:36 AM
llvm/tools/llvm-dwarfdump/SectionSizes.cpp
35

Why 13 here? I'd think it would only need to have a minimum of the size of "SECTION" + 2 or so.

49

The dashes under "SECTION" should be increased in number based on the column width.

98

collectObjectSectionSizes is new code, so it should use the right signature from day 1 rather than waiting until a later change. In fact, D75727 can probably be committed first.

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
421

I just want to be clear: is this how clang-format formats this comment?

649–652

A thought: is there any reason we can't allow --statistics and --show-section-sizes in the same run?

djtodoro updated this revision to Diff 249304.Mar 10 2020, 3:56 AM

-Improve the output
-Support Mach-O
-Refactor the code

djtodoro marked 4 inline comments as done.Mar 10 2020, 4:02 AM

Sorry for taking so long, I have three more questions:

What does the output look like for fat Mach-O binaries? There should be some in the dsymutil test inputs, if you have trouble creating one.
What does the output look like for a static .a archive?

I have added the tests for this.

I'm wondering if we should include the *data* collected here in the JSON output of --statistics if we don't already.

We do not include, but I can split the methods from SectionSizes.cpp into a header file, and allow the Statistics part to using this as well. I think this fields are useful for that. WDYT?

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

Sure. The magic value was used within some other LLVM tool when printing the section name, that is why I put it like this.

80

Sure.

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
421

Yes.

649–652

The option should act as "pretty printing", I think we should not allow mixing the output with --statistics JSON output. But, we can include these numbers within --statistics output as well.

We do not include, but I can split the methods from SectionSizes.cpp into a header file, and allow the Statistics part to using this as well. I think this fields are useful for that. WDYT?

Yes, I think the size data makes a lot of sense for --statistics.

djtodoro updated this revision to Diff 249586.Mar 11 2020, 4:47 AM

-Calculate the section sizes for --statistics

djtodoro updated this revision to Diff 249587.Mar 11 2020, 4:49 AM

-Minor update (using the const ref)

I think we're almost there!

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

compute this as a constexpr?

56

Shouldn't this be in libObject instead?

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

-*- C++ -*-

this is a header file.

23

Is the StringRef the section name? These are so small that we could use an llvm::StringMap instead.

aprantl added inline comments.Mar 11 2020, 9:37 AM
llvm/test/tools/llvm-dwarfdump/X86/archive_section_sizes.test
47 ↗(On Diff #249587)

It would be cute to also print the cumulative summary of all files in the archive, but this is not something that needs to be done to get this patch landed.

jhenderson added inline comments.Mar 12 2020, 3:28 AM
llvm/test/tools/llvm-dwarfdump/X86/archive_section_sizes.test
1 ↗(On Diff #249587)

As this test is specifically for mach fat archives, perhaps worth renaming the test "fat_archive_section_sizes.test" or possibly "macho_archive_section_sizes.test".

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

length -> width

Your comment suggests that the size should include the "+ 2", and the initial MaxWidth does so, but then you don't include it in the loop, and the rest of the code adds the 2 on, so I think there's an inconsistency here.

26

I'd go with "MaxNameWidth".

30

I would suggest that the start of "SECTION" lines up with the start of the section names, to match the sizes column.

57–61

I think the naming here needs to be format specific unfortunately, as "__debug" is not a debug section for ELF. Perhaps not worth worrying about though...?

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

the found debug sections

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
649–652

Okay, sounds reasonable.

Since one of these ifs already uses braces, I recommend all within this bit do for consistency.

djtodoro updated this revision to Diff 249953.Mar 12 2020, 9:07 AM

-Addressing comments
-Improving the output

djtodoro marked 3 inline comments as done.Mar 12 2020, 9:12 AM

I tested this output on bigger files, i.e. gdb:

llvm-dwarfdump --show-section-sizes gdb
----------------------------------------------------
file: gdb
----------------------------------------------------
SECTION         SIZE
--------------  --------
.debug_info     74863947 (63.71%)
.debug_loc      15324074 (13.04%)
.debug_ranges   2709232  (2.31%)
.debug_aranges  66432    (0.06%)
.debug_abbrev   2103393  (1.79%)
.debug_line     4517257  (3.84%)
.debug_str      8194356  (6.97%)

 Total Size: 107778691  (91.72%)
 Total File Size: 117510032
----------------------------------------------------
llvm/tools/llvm-dwarfdump/SectionSizes.cpp
15

I have change the function a bit, so I think we do not need the "constexpr" anymore.

15

Your comment suggests that the size should include the "+ 2", and the initial MaxWidth does so, but then you don't include it in the loop, and the rest of the code adds the 2 on, so I think there's an inconsistency here.

Sorry, I am not sure I follow :) Maybe my comment is not clear enough? The "+2" refers only for the minimum width.

56

Since the ELF does not consider the '__debug*' as debug sections, I would use this locally.

djtodoro marked an inline comment as done.Mar 12 2020, 9:14 AM
djtodoro added inline comments.
llvm/tools/llvm-dwarfdump/SectionSizes.cpp
57–61

I agree, that is why I would keep this function local to llvm-dwarfdump.

aprantl added inline comments.Mar 12 2020, 9:32 AM
llvm/tools/llvm-dwarfdump/SectionSizes.cpp
56

Wait, so now I'm confused :-)

__debug* is the Mach-O section name for the ELF .debug* sections (because of different naming rules and constraints). I don't think you would ever have a __debug* section in a n ELF object. So this should be implementable as a virtual method in ObjectFile, right?

djtodoro marked an inline comment as done.Mar 13 2020, 12:50 AM
djtodoro added inline comments.
llvm/tools/llvm-dwarfdump/SectionSizes.cpp
56

Oh right, we do not need to implement it within ElfObjectFile, that makes sense :)

djtodoro updated this revision to Diff 250140.Mar 13 2020, 12:51 AM

-Addressing comments

jhenderson added inline comments.Mar 16 2020, 3:17 AM
llvm/include/llvm/Object/ObjectFile.h
126 ↗(On Diff #250140)

is a debug

278 ↗(On Diff #250140)

Perhaps this interface extension can be pulled into a separate review. You could then modify llvm-objcopy in that same patch to make use of it.

llvm/lib/Object/ObjectFile.cpp
94 ↗(On Diff #250140)

I think this function needs to be implemented separately for ELF/Mach-O etc. For ELF objects, we don't want __debug counting as a debug section, whilst for Mach-O we don't want .debug. And for COFF etc, we don't want either probably!

It should be easy to make this a virtual function overridden by the object file types.

llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test
7 ↗(On Diff #250140)

I'd expect the dashes to go to the closing parenthesis for the percentage.

15–16 ↗(On Diff #250140)

For consistency, it probably makes sense to have these formatted the same as the section names, and have the Section column width take account of this too. I don't feel strongly about this though, so if you prefer it this way, that's fine.

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

In another test file possibly, you should test the behaviour where there are no debug sections. Maybe also where all sections are empty? Not sure if that's really important though.

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

I don't think you need to pass in SectionNameColPrintName for this purpose. Probably simpler would be to factor out a simple local string constant const StringRef = "Section".

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.

28

Let's make this consistent with my suggestions above, and use the column title as the minimum width.

31

You need to allow for the percentage part of the size too, I think.

40

Typo in variable name.

113–114

Clarify what "it's" is here (i.e. use "If the input file is an archive")

Also, I'd move the TODO below the function call to print.

llvm/tools/llvm-dwarfdump/SectionSizes.h
18–19

I don't think it's good form to put using namespace in a header file. Wrap the whole thing inside namespace llvm etc.

21

No need for the llvm::

35

Maybe worth calling this getNameColumnWidth and updating the comment accordingly.

39

Similar coment to the above. How about simply getSizeColumnWidth?

djtodoro updated this revision to Diff 250723.Mar 17 2020, 4:41 AM

-Addressing comments

djtodoro marked 6 inline comments as done.Mar 17 2020, 4:54 AM

@jhenderson Thanks for the review again!

llvm/include/llvm/Object/ObjectFile.h
278 ↗(On Diff #250140)

Done with the D76276.

But I have not refactor the llvm-objcopy part, yet. That could be a separate patch.

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