[llvm-objcopy][COFF] Fix section name encoding

[npmiller]

Summary

This patch is bringing the section name encoding of llvm-objcopy to the same level as what's done in llvm/lib/MC/WinCOFFObjectWriter.cpp, the base64 encoding code is taken from there.

The section name encoding for llvm-objcopy had two main issues, the first is that the size used for the snprintf in the original code is incorrect because snprintf adds a null byte, so this code was only able to encode offsets of 6 digits, /, \0 and 6 digits of the offset, rather than the 7 digits it should support.

And the second part is that it didn't support the base64 encoding for offsets larger than 7 digits.

This issue specifically showed up when using the clang-offload-bundler with a binary containing a lot of symbols/sections, since it uses llvm-objcopy to add the sections containing the offload code.

This patch may need some modifications and/or follow up.

It would be good to de-duplicate some of this code, at least the base64 encoding code, there's a base64 encoding function in support however it encodes bytes, rather than re-interpret a number into base64, but I'm not sure where it would go best.

And I'm also a little unsure on how to approach testing for this, yaml2obj, however it is also missing base64 encoding which should definitely be added, but even then the yaml file ends up very large to be able to test all the different encodings. The MC counterpart of this uses llvm-mc and assembler macros to generate the very large string tables, would it be okay to do the same in llvm-objcopy testing?

[mstorsjo]

llvm/tools/llvm-objcopy/COFF/Writer.cpp

173

Wouldn't this end up overwriting the first byte following S.Header.Name? (It might be benign to do that in practice here, but nevertheless, it would be better to not write outside of the intended struct fields.)

[npmiller]

llvm/tools/llvm-objcopy/COFF/Writer.cpp

173

No because this is no writing to the struct field but to an intermediary string str. The correct amount of data is memcpy'd into the struct field from str a bit further down.

[mstorsjo]

llvm/tools/llvm-objcopy/COFF/Writer.cpp

173

Oh, right, I see! Sorry for the confusion.

[jhenderson]

And I'm also a little unsure on how to approach testing for this, yaml2obj, however it is also missing base64 encoding which should definitely be added, but even then the yaml file ends up very large to be able to test all the different encodings. The MC counterpart of this uses llvm-mc and assembler macros to generate the very large string tables, would it be okay to do the same in llvm-objcopy testing?

If it's impractical to use yaml2obj, using llvm-mc is fine. Just make sure the appropriate REQUIRES: directive is included for the target.

llvm/tools/llvm-objcopy/COFF/Writer.cpp

159

Please make sure comments follow LLVM coding standards with upper-case first letter, and period at the end.

163

LLVM coding style uses UpperCamelCase for variable names. Please fix your variable names. Also, you can zero-initialise, without the need for memset.

[npmiller]

This updated patch adds a test using llvm-mc to check the section name encoding, it is inspired from the MC test for this feature llvm/test/MC/COFF/section-name-encoding.s, but tweaked for llvm-objcopy.

It also fixes up the comment formatting, variable naming and zero-initialization as suggested.

[jhenderson]

llvm/test/tools/llvm-objcopy/COFF/section-name-encoding.s

1–2 ↗

(On Diff #405003)

llvm-objcopy generally uses # and ## in lit test files, the former for test directives, like RUN lines, and FileCheck patterns, the latter for pure comments.

(If llvm-mc doesn't like # as a comment marker, then I'd use //// still for true comments, to distinguish them from test directives).

llvm/tools/llvm-objcopy/COFF/Writer.cpp

126

In terms of avoiding duplicated code, I think you should, in a separate prerequisite patch, move the duplicate code from WinCOFFObjectWriter.cpp to somewhere like the BinaryFormat or Object library. It's not immediately clear to me which is more appropriate though.

172

I'd use static_cast rather than a C-style cast, whilst you're changing this line.

[npmiller]

Comment prefixes in the test have been updated as suggested.

I've also filed a separate parent revision to move the section name encoding code from llvm/lib/MC/WinCOFFObjectWriter.cpp into BinaryFormat, and updated this patch to use that instead.

See: https://reviews.llvm.org/D118793

[mstorsjo]

@jhenderson Can you have another look at this? I ran into the issue that this patch is fixing (discussion in https://github.com/msys2/MINGW-packages/pull/10555#issuecomment-1046014913), and if at all possible, I'd like to have the fix for this issue in the 14.0.x branch (as we're only just past rc1, there should still be time for valuable bug fixes IMO).

What do you think about that? Alternatively, if this feels too big to cherrypick (IMO it isn't), I could fix the simplest part of the issue with a smaller patch like this:

diff --git a/llvm/lib/ObjCopy/COFF/Writer.cpp b/llvm/lib/ObjCopy/COFF/Writer.cpp
index cbd0e4261238..c7928b009b5d 100644
--- a/llvm/lib/ObjCopy/COFF/Writer.cpp
+++ b/llvm/lib/ObjCopy/COFF/Writer.cpp
@@ -130,8 +130,10 @@ size_t COFFWriter::finalizeStringTable() {
   for (auto &S : Obj.getMutableSections()) {
     memset(S.Header.Name, 0, sizeof(S.Header.Name));
     if (S.Name.size() > COFF::NameSize) {
-      snprintf(S.Header.Name, sizeof(S.Header.Name), "/%d",
+      char Buf[COFF::NameSize + 1]{};
+      snprintf(Buf, sizeof(Buf), "/%d",
                (int)StrTabBuilder.getOffset(S.Name));
+      memcpy(S.Header.Name, Buf, COFF::NameSize);
     } else {
       memcpy(S.Header.Name, S.Name.data(), S.Name.size());
     }

(This would fix the bug I ran into, where the bug appears with string tables over 10^6 bytes. This minimal patch makes things work with string tables up to 10^7 bytes - while the patch under review here handles it even further than that.)

[jhenderson]

LGTM. Sorry, not sure how this fell off my radar. I'm happy for it to be picked too. @mstorsjo, you are good with this patch, I take it?

[mstorsjo]

In D118692#3334895, @jhenderson wrote:

LGTM. Sorry, not sure how this fell off my radar. I'm happy for it to be picked too. @mstorsjo, you are good with this patch, I take it?

Yep, it looks good to me too. I'll go ahead and land D118793 and this one then, and then file the backport ticket. (Re backporting, the code has been moved between llvm/tools/llvm-objcopy and llvm/lib/ObjCopy between the releases/14.x and main branches, but git cherry-pick seems to handle it fine regardless - let's hope it works in the backport testing bot too.)

[RKSimon]

This is failing: https://lab.llvm.org/buildbot/#/builders/121/builds/16742

FAILED: lib/libLLVMObjCopy.so.15git 
: && /usr/bin/c++ -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG  -Wl,-z,defs -Wl,-z,nodelete   -Wl,-rpath-link,/home/buildbots/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/./lib  -Wl,--gc-sections -shared -Wl,-soname,libLLVMObjCopy.so.15git -o lib/libLLVMObjCopy.so.15git lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/Archive.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/ObjCopy.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/ConfigManager.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/COFF/COFFObjcopy.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/COFF/Object.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/COFF/Reader.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/COFF/Writer.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/ELF/ELFObjcopy.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/ELF/Object.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/MachO/MachOObjcopy.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/MachO/MachOReader.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/MachO/MachOWriter.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/MachO/MachOLayoutBuilder.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/MachO/Object.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/wasm/Object.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/wasm/Reader.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/wasm/Writer.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/wasm/WasmObjcopy.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMObject.so.15git  lib/libLLVMMC.so.15git  lib/libLLVMSupport.so.15git  -Wl,-rpath-link,/home/buildbots/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/lib && :
lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/COFF/Writer.cpp.o: In function `llvm::objcopy::coff::COFFWriter::finalizeStringTable() [clone .localalias.6]':
Writer.cpp:(.text._ZN4llvm7objcopy4coff10COFFWriter19finalizeStringTableEv+0x1ec): undefined reference to `llvm::COFF::encodeSectionName(char*, unsigned long)'

[mstorsjo]

In D118692#3335292, @RKSimon wrote:

This is failing: https://lab.llvm.org/buildbot/#/builders/121/builds/16742

FAILED: lib/libLLVMObjCopy.so.15git 
: && /usr/bin/c++ -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG  -Wl,-z,defs -Wl,-z,nodelete   -Wl,-rpath-link,/home/buildbots/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/./lib  -Wl,--gc-sections -shared -Wl,-soname,libLLVMObjCopy.so.15git -o lib/libLLVMObjCopy.so.15git lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/Archive.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/ObjCopy.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/ConfigManager.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/COFF/COFFObjcopy.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/COFF/Object.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/COFF/Reader.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/COFF/Writer.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/ELF/ELFObjcopy.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/ELF/Object.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/MachO/MachOObjcopy.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/MachO/MachOReader.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/MachO/MachOWriter.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/MachO/MachOLayoutBuilder.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/MachO/Object.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/wasm/Object.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/wasm/Reader.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/wasm/Writer.cpp.o lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/wasm/WasmObjcopy.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMObject.so.15git  lib/libLLVMMC.so.15git  lib/libLLVMSupport.so.15git  -Wl,-rpath-link,/home/buildbots/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/lib && :
lib/ObjCopy/CMakeFiles/LLVMObjCopy.dir/COFF/Writer.cpp.o: In function `llvm::objcopy::coff::COFFWriter::finalizeStringTable() [clone .localalias.6]':
Writer.cpp:(.text._ZN4llvm7objcopy4coff10COFFWriter19finalizeStringTableEv+0x1ec): undefined reference to `llvm::COFF::encodeSectionName(char*, unsigned long)'

Sorry about that - if you happen to have such a build available, does this diff help?

diff --git a/llvm/lib/ObjCopy/CMakeLists.txt b/llvm/lib/ObjCopy/CMakeLists.txt
index 1e516394c74a..ec1160e331c9 100644
--- a/llvm/lib/ObjCopy/CMakeLists.txt
+++ b/llvm/lib/ObjCopy/CMakeLists.txt
@@ -64,6 +64,7 @@ add_llvm_component_library(LLVMObjCopy
   intrinsics_gen

   LINK_COMPONENTS
+  BinaryFormat
   Object
   Support
   MC

[svenvh]

In D118692#3335370, @mstorsjo wrote:

Sorry about that - if you happen to have such a build available, does this diff help?

diff --git a/llvm/lib/ObjCopy/CMakeLists.txt b/llvm/lib/ObjCopy/CMakeLists.txt
index 1e516394c74a..ec1160e331c9 100644
--- a/llvm/lib/ObjCopy/CMakeLists.txt
+++ b/llvm/lib/ObjCopy/CMakeLists.txt
@@ -64,6 +64,7 @@ add_llvm_component_library(LLVMObjCopy
   intrinsics_gen

   LINK_COMPONENTS
+  BinaryFormat
   Object
   Support
   MC

I have just committed this exact same patch, which did fix the shared libs build issue for me locally.

[mstorsjo]

In D118692#3335374, @svenvh wrote:

In D118692#3335370, @mstorsjo wrote:

Sorry about that - if you happen to have such a build available, does this diff help?

diff --git a/llvm/lib/ObjCopy/CMakeLists.txt b/llvm/lib/ObjCopy/CMakeLists.txt
index 1e516394c74a..ec1160e331c9 100644
--- a/llvm/lib/ObjCopy/CMakeLists.txt
+++ b/llvm/lib/ObjCopy/CMakeLists.txt
@@ -64,6 +64,7 @@ add_llvm_component_library(LLVMObjCopy
   intrinsics_gen

   LINK_COMPONENTS
+  BinaryFormat
   Object
   Support
   MC

I have just committed this exact same patch, which did fix the shared libs build issue for me locally.

Thanks!

llvm/tools/llvm-objcopy/COFF/Writer.h

class COFFWriter {
   size_t FileAlignment;
   size_t SizeOfInitializedData;
   StringTableBuilder StrTabBuilder;
 
   template <class SymbolTy> std::pair<size_t, size_t> finalizeSymbolTable();
   Error finalizeRelocTargets();
   Error finalizeSymbolContents();
   void layoutSections();
-  size_t finalizeStringTable();
+  Expected<size_t> finalizeStringTable();
 
   Error finalize(bool IsBigObj);
 
   void writeHeaders(bool IsBigObj);
   void writeSections();
   template <class SymbolTy> void writeSymbolStringTables();
 
   Error write(bool IsBigObj);

llvm/tools/llvm-objcopy/COFF/Writer.cpp

for (auto &S : Obj.getMutableSections()) {
     FileSize += S.Relocs.size() * sizeof(coff_relocation);
     FileSize = alignTo(FileSize, FileAlignment);
 
     if (S.Header.Characteristics & IMAGE_SCN_CNT_INITIALIZED_DATA)
       SizeOfInitializedData += S.Header.SizeOfRawData;
   }
 }
 
-size_t COFFWriter::finalizeStringTable() {
+// Maximum offsets for different string table entry encodings.
+enum : unsigned { Max7DecimalOffset = 9999999U };
+enum : uint64_t { MaxBase64Offset = 0xFFFFFFFFFULL }; // 64^6, including 0
+
+// Encode a string table entry offset in base 64, padded to 6 chars, and
+// prefixed with a double slash: '//AAAAAA', '//AAAAAB', ...
+// Buffer must be at least 8 bytes large. No terminating null appended.
+static void encodeBase64StringEntry(char *Buffer, uint64_t Value) {
+  assert(Value > Max7DecimalOffset && Value <= MaxBase64Offset &&
+         "Illegal section name encoding for value");
+
+  static const char Alphabet[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+                                 "abcdefghijklmnopqrstuvwxyz"
+                                 "0123456789+/";
+
+  Buffer[0] = '/';
+  Buffer[1] = '/';
+
+  char *Ptr = Buffer + 7;
+  for (unsigned i = 0; i < 6; ++i) {
+    unsigned Rem = Value % 64;
+    Value /= 64;
+    *(Ptr--) = Alphabet[Rem];
+  }
+}
+
+Expected<size_t> COFFWriter::finalizeStringTable() {
   for (const auto &S : Obj.getSections())
     if (S.Name.size() > COFF::NameSize)
       StrTabBuilder.add(S.Name);
 
   for (const auto &S : Obj.getSymbols())
     if (S.Name.size() > COFF::NameSize)
       StrTabBuilder.add(S.Name);
 
   StrTabBuilder.finalize();
 
   for (auto &S : Obj.getMutableSections()) {
     memset(S.Header.Name, 0, sizeof(S.Header.Name));
-    if (S.Name.size() > COFF::NameSize) {
-      snprintf(S.Header.Name, sizeof(S.Header.Name), "/%d",
-               (int)StrTabBuilder.getOffset(S.Name));
-    } else {
+    if (S.Name.size() <= COFF::NameSize) {
+      // short names can go in the field directly
       memcpy(S.Header.Name, S.Name.data(), S.Name.size());
+    } else {
+      // longer names need to go in the string table
+      char str[sizeof(S.Header.Name) + 1];
+      memset(str, 0, sizeof(S.Header.Name) + 1);
+
+      // offset of the section name in the string table
+      size_t offset = StrTabBuilder.getOffset(S.Name);
+
+      if (offset <= Max7DecimalOffset) {
+        // offsets with 7 digits or less are encoded with a / then the number in
+        // ASCII, the snprintf size needs to be one more than the field size as
+        // it adds a null termination character
+        snprintf(str, sizeof(S.Header.Name) + 1, "/%d", (int)offset);
+      } else if (offset <= MaxBase64Offset) {
+        // offsets with more than 7 digits are encoded with two / then the
+        // number in base64
+        encodeBase64StringEntry(str, offset);
+      } else {
+        // the section offset is too large to be correctly encoded
+        return createStringError(object_error::invalid_section_index,
+                                 "COFF string table is greater than 64GB, "
+                                 "unable to encode section name offset");
+      }
+
+      // copy the correctly encoded section name into the header
+      memcpy(S.Header.Name, str, sizeof(S.Header.Name));
     }
   }
   for (auto &S : Obj.getMutableSymbols()) {
     if (S.Name.size() > COFF::NameSize) {
       S.Sym.Name.Offset.Zeroes = 0;
       S.Sym.Name.Offset.Offset = StrTabBuilder.getOffset(S.Name);
     } else {
       strncpy(S.Sym.Name.ShortName, S.Name.data(), COFF::NameSize);
if (!Obj.getSections().empty()) {
                   Obj.PeHeader.SectionAlignment);
     }
 
     // If the PE header had a checksum, clear it, since it isn't valid
     // any longer. (We don't calculate a new one.)
     Obj.PeHeader.CheckSum = 0;
   }
 
-  size_t StrTabSize = finalizeStringTable();
+  Expected<size_t> StrTabSizeOrErr = finalizeStringTable();
+  if (!StrTabSizeOrErr)
+    return StrTabSizeOrErr.takeError();
+
+  size_t StrTabSize = *StrTabSizeOrErr;
 
   size_t PointerToSymbolTable = FileSize;
   // StrTabSize <= 4 is the size of an empty string table, only consisting
   // of the length field.
   if (SymTabSize == 0 && StrTabSize <= 4 && Obj.IsPE) {
     // For executables, don't point to the symbol table and skip writing
     // the length field, if both the symbol and string tables are empty.
     PointerToSymbolTable = 0;