fixup! [llvm-objcopy] Error with relevant message when adding invalid notes

Author: serge-sans-paille

llvm/docs/CommandGuide/llvm-objcopy.rst

@@ -512,6 +512,12 @@ them.
  specified format. See `SUPPORTED FORMATS`_ for a list of valid ``<format>``
  values.
 
+.. option:: --verify-note-sections, --no-verify-note-sections
+
+ When adding note sections, verify if the section format is valid. Defaults to
+ ``--verify-note-sections``
+
+
 .. option:: --weaken-symbol <symbol>, -W
 
  Mark global symbols named ``<symbol>`` as weak symbols in the output. Can

llvm/include/llvm/ObjCopy/CommonConfig.h

@@ -258,7 +258,6 @@ struct CommonConfig {
   bool StripNonAlloc = false;
   bool StripSections = false;
   bool StripUnneeded = false;
-  bool VerifyNoteSections = true;
   bool Weaken = false;
   bool DecompressDebugSections = false;
 

llvm/include/llvm/ObjCopy/ELF/ELFConfig.h

@@ -30,6 +30,7 @@ struct ELFConfig {
   bool AllowBrokenLinks = false;
   bool KeepFileSymbols = false;
   bool LocalizeHidden = false;
+  bool VerifyNoteSections = true;
 };
 
 } // namespace objcopy

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

@@ -653,7 +653,7 @@ static Error verifyNoteSection(StringRef Name, endianness Endianness,
   uint32_t NameSizeValue = *reinterpret_cast<const uint32_t *>(NameSize.data());
   uint32_t DescSizeValue = *reinterpret_cast<const uint32_t *>(DescSize.data());
 
-  if (Endianness != llvm::endianness::native) {
+  if (Endianness != endianness::native) {
     NameSizeValue = byteswap(NameSizeValue);
     DescSizeValue = byteswap(DescSizeValue);
   }
@@ -737,7 +737,7 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
           Obj.addSection<OwnedDataSection>(Name, Data);
       if (Name.starts_with(".note") && Name != ".note.GNU-stack") {
         NewSection.Type = SHT_NOTE;
-        if (Config.VerifyNoteSections)
+        if (ELFConfig.VerifyNoteSections)
           return verifyNoteSection(Name, E, Data);
       }
       return Error::success();

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

@@ -1,34 +1,46 @@
 ## Verify that --add-section warns on invalid note sections.
 
 # Add [namesz, descsz, type, name, desc] for a build id.
-#
+
+## Notes should be padded on 8 bits
 # RUN: echo -e -n "\x04\x00\x00\x00" >  %t-miss-padding-note.bin
 # RUN: echo -e -n "\x07\x00\x00\x00" >> %t-miss-padding-note.bin
 # RUN: echo -e -n "\x03\x00\x00\x00" >> %t-miss-padding-note.bin
 # RUN: echo -e -n "GNU\x00"          >> %t-miss-padding-note.bin
 # RUN: echo -e -n "\x0c\x0d\x0e"     >> %t-miss-padding-note.bin
-#
+
+## The namesz field bit is incorrect
 # RUN: echo -e -n "\x08\x00\x00\x00" >  %t-invalid-size-note.bin
 # RUN: echo -e -n "\x07\x00\x00\x00" >> %t-invalid-size-note.bin
 # RUN: echo -e -n "\x03\x00\x00\x00" >> %t-invalid-size-note.bin
 # RUN: echo -e -n "GNU\x00"          >> %t-invalid-size-note.bin
 # RUN: echo -e -n "\x0c\x0d\x0e\x00" >> %t-invalid-size-note.bin
 
+## Missing type field
 # RUN: echo -e -n "\x08\x00\x00\x00" >  %t-short-note.bin
 # RUN: echo -e -n "\x07\x00\x00\x00" >> %t-short-note.bin
 
 # RUN: yaml2obj %s -o %t.o
+
+## test each invalid note
 # RUN: not llvm-objcopy --add-section=.note.miss.padding=%t-miss-padding-note.bin %t.o %t-with-note.o 2>&1 | FileCheck --check-prefix=CHECK-MISS-PADDING %s
+# CHECK-MISS-PADDING: .note.miss.padding data size must be a multiple of 4 bytes
+
 # RUN: not llvm-objcopy --add-section=.note.invalid.size=%t-invalid-size-note.bin %t.o %t-with-note.o 2>&1 | FileCheck --check-prefix=CHECK-INVALID-SIZE %s
+# CHECK-INVALID-SIZE: .note.invalid.size data size is incompatible with the content of the name and description size fields: expecting 28, found 20
+
 # RUN: not llvm-objcopy --add-section=.note.short=%t-short-note.bin %t.o %t-with-note.o 2>&1 | FileCheck --check-prefix=CHECK-SHORT %s
+# CHECK-SHORT: .note.short data must be either empty or at least 12 bytes long
+
+## test that verification can be disabled
 # RUN: llvm-objcopy --add-section=.note.short=%t-short-note.bin --no-verify-note-sections %t.o %t-with-note.o
 
+## test that last argument has precedence
+# RUN: llvm-objcopy --add-section=.note.short=%t-short-note.bin --verify-note-section --no-verify-note-sections %t.o %t-with-note.o
+
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
 
-# CHECK-MISS-PADDING: .note.miss.padding data size must be a multiple of 4 bytes
-# CHECK-INVALID-SIZE: .note.invalid.size data size is incompatible with the content of the name and description size fields: expecting 28, found 20
-# CHECK-SHORT: .note.short data must be either empty or at least 12 bytes long

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

@@ -949,8 +949,12 @@ objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
                              ? DiscardType::All
                              : DiscardType::Locals;
   }
-  Config.VerifyNoteSections =
-      !InputArgs.hasArg(OBJCOPY_no_verify_note_sections);
+
+  const bool ShouldVerifyNoteSectionsByDefault = true;
+  ELFConfig.VerifyNoteSections = InputArgs.hasFlag(
+      OBJCOPY_verify_note_sections, OBJCOPY_no_verify_note_sections,
+      ShouldVerifyNoteSectionsByDefault);
+
   Config.OnlyKeepDebug = InputArgs.hasArg(OBJCOPY_only_keep_debug);
   ELFConfig.KeepFileSymbols = InputArgs.hasArg(OBJCOPY_keep_file_symbols);
   MachOConfig.KeepUndefined = InputArgs.hasArg(OBJCOPY_keep_undefined);

llvm/tools/llvm-objcopy/ObjcopyOpts.td

@@ -1,5 +1,11 @@
 include "CommonOpts.td"
 
+multiclass B<string name, string help1, string help2> {
+  def NAME: Flag<["--"], name>, HelpText<help1>;
+  def no_ # NAME: Flag<["--"], "no-" # name>, HelpText<help2>;
+}
+
+
 defm binary_architecture
     : Eq<"binary-architecture", "Ignored for compatibility">;
 def B : JoinedOrSeparate<["-"], "B">,
@@ -176,8 +182,9 @@ def G : JoinedOrSeparate<["-"], "G">,
         Alias<keep_global_symbol>,
         HelpText<"Alias for --keep-global-symbol">;
 
-def no_verify_note_sections : Flag<["--"], "no-verify-note-sections">,
-                        HelpText<"Disable note section validation">;
+defm verify_note_sections : B<"verify-note-sections",
+                              "Validate note sections",
+                              "Don't validate note sections">;
 
 defm keep_global_symbols
     : Eq<"keep-global-symbols",