Page MenuHomePhabricator

[llvm-objcopy] -O binary: consider SHT_NOBITS sections to be empty
ClosedPublic

Authored by pattop on Jan 27 2021, 3:35 PM.

Details

Summary

This is consistent with BFD objcopy.

Previously llvm objcopy would allocate space for SHT_NOBITS sections
often resulting in enormous binary files.

New test case (binary-paddr.test %t6).

Diff Detail

Event Timeline

pattop created this revision.Jan 27 2021, 3:35 PM
pattop requested review of this revision.Jan 27 2021, 3:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2021, 3:35 PM

I did not add Sec.Type != SHT_NOBITS && in D79229 because I did not find a use case for this rule... If yours is a real usage, I think it is fine.

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
214

I'll use a smaller address. If something goes off, this would not create a 260MB output.

Yes, this came from a real use case.

Thanks!

I forgot to mention that I do not have commit access. If this is acceptable someone else will need to commit on my behalf. Thanks again.

pattop added inline comments.Jan 31 2021, 2:23 PM
llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
214

Do you want me to update the patch with this changed, or are you happy to adjust it when committing?

MaskRay added inline comments.Jan 31 2021, 2:33 PM
llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
214

Please update it.

I am happy with the patch, just wanted to give others some time reviewing it as well.

pattop updated this revision to Diff 320392.Jan 31 2021, 9:15 PM

Use a smaller address in test case. This means that if something goes wrong we don't try to make a huge file.

pattop marked 2 inline comments as done.Jan 31 2021, 9:16 PM
jhenderson accepted this revision.Feb 1 2021, 2:39 AM

I haven't verified the correctness of this versus GNU objcopy, but assuming the two behave identically after this patch, this is fine by me. I'd like someone else to commit though, as I have too much else on my plate currently.

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
214

This could be as small as 0x4000 maybe?

This revision is now accepted and ready to land.Feb 1 2021, 2:39 AM
pattop updated this revision to Diff 320613.Feb 1 2021, 2:53 PM

Use smaller address in test case.

pattop marked an inline comment as done.Feb 1 2021, 2:53 PM
MaskRay added inline comments.Feb 1 2021, 3:00 PM
llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
190

I'll add --ignore-case and commit.

MaskRay accepted this revision.Feb 1 2021, 3:00 PM
This revision was landed with ongoing or failed builds.Feb 1 2021, 3:01 PM
This revision was automatically updated to reflect the committed changes.