[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

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

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

502

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

/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

This passes for me on my Windows machine.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

512

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.

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

+# 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
+# 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
+
+# NOTE:      .note.foo       NOTE
+# GNU-STACK: .note.GNU-stack PROGBITS

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);
       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));
+      auto &NewSection = Obj.addSection<OwnedDataSection>(
+          SecName, ArrayRef<uint8_t>(BufPtr, BufSize));
+      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;