[PATCH] D56570: [llvm-objcopy] Use SHT_NOTE for added note sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

In D56570#1354701 <https://reviews.llvm.org/D56570#1354701>, @rupprecht wrote:

> In D56570#1353888 <https://reviews.llvm.org/D56570#1353888>, @jhenderson wrote:
>
> > 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.



================
Comment at: test/tools/llvm-objcopy/ELF/add-note.test:5
+# Add [namesz, descsz, type, name, desc] for a build id.
+# RUN: echo -en "\x04\x00\x00\x00" >  %t-note.bin
+# RUN: echo -en "\x10\x00\x00\x00" >> %t-note.bin
----------------
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.


================
Comment at: test/tools/llvm-objcopy/ELF/add-section-special.test:2
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-objcopy --add-section=.note.foo=/dev/null %t.o %t-regular-note.o
+# RUN: llvm-objcopy --add-section=.note.GNU-stack=/dev/null %t.o %t-gnu-stack.o
----------------
rupprecht wrote:
> alexshap wrote:
> > just to double check - wouldn't this test have troubles on Windows ?
> /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.
This passes for me on my Windows machine.


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:510
+      std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
+      const auto *BufPtr =
+          reinterpret_cast<const uint8_t *>(Buf->getBufferStart());
----------------
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56570/new/

https://reviews.llvm.org/D56570