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

[rupprecht]

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

[alexander-shaposhnikov]

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)

[jhenderson]

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]

• Add test for parsing an actual note added
• Un-autoify rest of the --add-section handling block

[rupprecht]

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

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]

In D56570#1354701, @rupprecht wrote:

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

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.

[rupprecht]

• 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]

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

llvm/trunk/test/tools/llvm-objcopy/ELF/add-note.test

+# Verify that --add-section can be used to add a note section which is
+# successfully interpreted by tools that read notes.
+
+# Add [namesz, descsz, type, name, desc] for a build id.
+# RUN: echo -e -n "\x04\x00\x00\x00" >  %t-note.bin
+# RUN: echo -e -n "\x10\x00\x00\x00" >> %t-note.bin
+# RUN: echo -e -n "\x03\x00\x00\x00" >> %t-note.bin
+# RUN: echo -e -n "GNU\x00"          >> %t-note.bin
+# RUN: echo -e -n "\x00\x01\x02\x03" >> %t-note.bin
+# RUN: echo -e -n "\x04\x05\x06\x07" >> %t-note.bin
+# RUN: echo -e -n "\x08\x09\x0a\x0b" >> %t-note.bin
+# RUN: echo -e -n "\x0c\x0d\x0e\x0f" >> %t-note.bin
+
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-objcopy --add-section=.note.gnu.build-id=%t-note.bin %t.o %t-with-note.o
+# RUN: llvm-readobj --notes %t-with-note.o | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+
+# CHECK:      Notes [
+# CHECK-NEXT:   NoteSection {
+# CHECK-NEXT:     Offset:
+# CHECK-NEXT:     Size:
+# CHECK-NEXT:     Note {
+# CHECK-NEXT:       Owner: GNU
+# CHECK-NEXT:       Data size: 0x10
+# CHECK-NEXT:       Type: NT_GNU_BUILD_ID
+# CHECK-NEXT:       Build ID: 000102030405060708090a0b0c0d0e0f
+# CHECK-NEXT:     }
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]

llvm/trunk/test/tools/llvm-objcopy/ELF/add-section-special.test

+# Check the properties of added sections.
+# By default, sections are SHT_PROGBITS, but .note sections (excluding
+# .note.GNU-stack) are SHT_NOTE sections.
+
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-objcopy --add-section=.foo=/dev/null %t.o %t-foo.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
+# RUN: llvm-readelf --sections %t-foo.o | FileCheck %s --check-prefix=NORMAL
+# RUN: llvm-readelf --sections %t-regular-note.o | FileCheck %s --check-prefix=NOTE
+# RUN: llvm-readelf --sections %t-gnu-stack.o | FileCheck %s --check-prefix=GNU-STACK
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+
+# NORMAL:    .foo            PROGBITS
+# NOTE:      .note.foo       NOTE
+# GNU-STACK: .note.GNU-stack PROGBITS

llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

for (auto &Sec : Obj.sections()) {
                       (SR.NewFlags.getValue() & ~PreserveMask);
         }
       }
     }
   }
 
   if (!Config.AddSection.empty()) {
     for (const auto &Flag : Config.AddSection) {
-      auto SecPair = Flag.split("=");
-      auto SecName = SecPair.first;
-      auto File = SecPair.second;
-      auto BufOrErr = MemoryBuffer::getFile(File);
+      std::pair<StringRef, StringRef> SecPair = Flag.split("=");
+      StringRef SecName = SecPair.first;
+      StringRef File = SecPair.second;
+      ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
+          MemoryBuffer::getFile(File);
       if (!BufOrErr)
         reportError(File, BufOrErr.getError());
-      auto Buf = std::move(*BufOrErr);
-      auto BufPtr = reinterpret_cast<const uint8_t *>(Buf->getBufferStart());
-      auto BufSize = Buf->getBufferSize();
-      Obj.addSection<OwnedDataSection>(SecName,
-                                       ArrayRef<uint8_t>(BufPtr, BufSize));
+      std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
+      ArrayRef<uint8_t> Data(
+          reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
+          Buf->getBufferSize());
+      OwnedDataSection &NewSection =
+          Obj.addSection<OwnedDataSection>(SecName, Data);
+      if (SecName.startswith(".note") && SecName != ".note.GNU-stack")
+        NewSection.Type = SHT_NOTE;
     }
   }
 
   if (!Config.DumpSection.empty()) {
     for (const auto &Flag : Config.DumpSection) {
       std::pair<StringRef, StringRef> SecPair = Flag.split("=");
       StringRef SecName = SecPair.first;
       StringRef File = SecPair.second;