Page MenuHomePhabricator

[llvm-objcopy] Use SHT_NOTE for added note sections.
ClosedPublic

Authored by rupprecht on Jan 10 2019, 5:21 PM.

Details

Summary

Fix llvm-objcopy to add .note sections as SHT_NOTEs. GNU objcopy overrides section flags for special sections. For .note sections (with the exception of .note.GNU-stack), SHT_NOTE is used.

Many other sections are special cased by libbfd, but .note is the only special section I can seem to find being used with objcopy --add-section.

See .note in context of the full list of special sections here: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/elf.c;h=eb3e1828e9c651678b95a1dcbc3b124783d1d2be;hb=HEAD#l2675

Diff Detail

Repository
rL LLVM

Event Timeline

rupprecht created this revision.Jan 10 2019, 5:21 PM

I'd probably modify the test (to make it work on Windows (if it doesn't)) (or add a comment why it works if it does), other than that (and one minor inline comment) this diff looks like the right fix to me.

test/tools/llvm-objcopy/ELF/add-section-special.test
2 ↗(On Diff #181197)

just to double check - wouldn't this test have troubles on Windows ?

tools/llvm-objcopy/ELF/ELFObjcopy.cpp
502 ↗(On Diff #181197)

side note: i think auto is overused here (lines 501 -508) (seems like the types here are not super-obvious)

I think this is probably okay, but I'm not 100% convinced. Is it possible to force the type of a section called ".note..." to something other than SHT_NOTE? I'm not sure if there's really a use case for this beyond testing though, so it's probably fine.

rupprecht marked 3 inline comments as done.
  • Add test for parsing an actual note added
  • Un-autoify rest of the --add-section handling block

Is it possible to force the type of a section called ".note..." to something other than SHT_NOTE?

I don't think so, but that's also kind of the point of this patch -- without it, there isn't a way to add a .note section and have it be an actual SHT_NOTE. I think .note sections are always expected to be SHT_NOTE (with the noted exception of .note.GNU-stack), so adding it as PROGBITS is weird.

test/tools/llvm-objcopy/ELF/add-section-special.test
2 ↗(On Diff #181197)

/dev/null seems to be widely used in tests, and llvm-lit explicitly handles /dev/null for "kAvoidDevNull" (which means Windows) here:
https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/llvm/utils/lit/lit/TestRunner.py#L873

I'll try to find someone to run this on Windows before I submit, but if not, I'll watch buildbots.

jhenderson requested changes to this revision.Jan 14 2019, 1:42 AM

Is it possible to force the type of a section called ".note..." to something other than SHT_NOTE?

I don't think so, but that's also kind of the point of this patch -- without it, there isn't a way to add a .note section and have it be an actual SHT_NOTE. I think .note sections are always expected to be SHT_NOTE (with the noted exception of .note.GNU-stack), so adding it as PROGBITS is weird.

Good point.

test/tools/llvm-objcopy/ELF/add-note.test
5 ↗(On Diff #181347)

I have a suspicion that I've seen a TODO in lit saying the built-in echo can only handle one switch.

This is backed up by the fact that the test fails on my Windows machine with an error from llvm-readobj saying that the note overflows its container.

test/tools/llvm-objcopy/ELF/add-section-special.test
2 ↗(On Diff #181197)

This passes for me on my Windows machine.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp
510 ↗(On Diff #181347)

This one you can use auto on for the whole thing, since it's in the reinterpret_cast.

Alternatively, construct the ArrayRef on this line, rather than in the addSection call later, and you only need to declare that, I believe?

In other words, I think this can be:

ArrayRef<uint8_t> BufPtr (reinterpret_cast<const uint8_t *>(Buf->getBufferStart()), BufSize);

You'd obviously need to switch this and the next line though.

This revision now requires changes to proceed.Jan 14 2019, 1:42 AM
rupprecht marked 3 inline comments as done.
  • Use -e -n instead of -en in shell tests
  • Inlined BufPtr
test/tools/llvm-objcopy/ELF/add-note.test
5 ↗(On Diff #181347)

Yes, looks like you're talking about this: https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/llvm/utils/lit/lit/TestRunner.py#L289

Switch to -e -n. Strange that it doesn't fail on my machine though -- it doesn't seem to be a windows specific limitation, but somehow -en works for me. It looks like lit does have an option to use the local shell (which would explain it), but that is not the default.

I could also check this bit in as a binary file, but I'd prefer this if possible.

jhenderson accepted this revision.Jan 15 2019, 2:11 AM

LGTM. I checked the test works for me after the modification.

This revision is now accepted and ready to land.Jan 15 2019, 2:11 AM
This revision was automatically updated to reflect the committed changes.