[COFF] Move section name encoding into BinaryFormat

[npmiller]

Summary

Large COFF section names are moved into the string table and the section header field is the offset into the string table encoded in ASCII for offset smaller than 7 digits and in base64 for larger offsets.

The operation of taking the string table offsets is done in a few places in the codebase, so it is helpful to move this operation into BinaryFormat so that it can be shared everywhere it's done.

So this patch takes the implementation of this operation from llvm/lib/MC/WinCOFFObjectWriter.cpp and moves it into BinaryFormat.

This is a prerequisite for https://reviews.llvm.org/D118692 that fixes an issue where llvm-objcopy was implementing this encoding operation incorrectly.

[rnk]

lgtm

[jhenderson]

Some nits from me. Otherwise, LGTM.

llvm/include/llvm/BinaryFormat/COFF.h

735

NameSize isn't referenced anywhere in this signature - I have to go a long way to find what it refers to. Perhaps replace NameSize with the NameSize constant value or something to that effect?

llvm/lib/BinaryFormat/CMakeLists.txt

15

Nit: filenames look to be in alphabetical order in this list, so move COFF.cpp much earlier.

llvm/lib/BinaryFormat/COFF.cpp

2

Nit: No need for the "*- C++-*" bit of this line - this is only for .h files where the extension doesn't signify the language.

[npmiller]

Fixed file order in CMakeLists.txt, removed unnecessary *- C++-* in .cpp source file.

For the NameSize comment I simply added the namespace, COFF::NameSize, which should make it clear that it's referring to the constant value.

[jhenderson]

Great, looks good.

[mstorsjo]

@npmiller If you don't have commit access, I can push this patch for you. Can you provide your preferred git author line, i.e. "Real Name <email@address>"?

[npmiller]

In D118793#3333621, @mstorsjo wrote:

@npmiller If you don't have commit access, I can push this patch for you. Can you provide your preferred git author line, i.e. "Real Name <email@address>"?

@mstorsjo yeah I don't have commit access so that would be great, for the git author line: "Nicolas Miller <[e-mail]>", thank you!

[mstorsjo]

In D118793#3334920, @npmiller wrote:

In D118793#3333621, @mstorsjo wrote:

@npmiller If you don't have commit access, I can push this patch for you. Can you provide your preferred git author line, i.e. "Real Name <email@address>"?

@mstorsjo yeah I don't have commit access so that would be great, for the git author line: "Nicolas Miller <[e-mail]>", thank you!

Awesome, I'll push it in a little while after building and running tests then.

Btw, lld/COFF/Writer.cpp also has another instance of code doing the same (OutputSection::writeHeaderTo), and it'd be nice to refactor that to use the same shared routine now. It's probably less common to have over 10^6 bytes of string table in an executable, but it's clearly possible. It has a slightly different set of bugs (it writes the null byte into the following field if it's up to 10^7 bytes, and doesn't implement the base64 alternative at all), so it'd be good to fix the same there too.

llvm/include/llvm/BinaryFormat/COFF.h

enum CodeViewIdentifiers {
   DEBUG_SECTION_MAGIC = 0x4,
   DEBUG_HASHES_SECTION_MAGIC = 0x133C9C5
 };
 
 inline bool isReservedSectionNumber(int32_t SectionNumber) {
   return SectionNumber <= 0;
 }
 
+/// Encode section name based on string table offset.
+/// The size of Out must be at least COFF::NameSize.
+bool encodeSectionName(char *Out, uint64_t Offset);
+
 } // End namespace COFF.
 } // End namespace llvm.
 
 #endif

llvm/lib/BinaryFormat/CMakeLists.txt

 add_llvm_component_library(LLVMBinaryFormat
   AMDGPUMetadataVerifier.cpp
+  COFF.cpp
   Dwarf.cpp
   ELF.cpp
   MachO.cpp
   Magic.cpp
   Minidump.cpp
   MsgPackDocument.cpp
   MsgPackDocumentYAML.cpp
   MsgPackReader.cpp
   MsgPackWriter.cpp
   Wasm.cpp
   XCOFF.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/BinaryFormat
 
   LINK_COMPONENTS
   Support
   )

llvm/lib/BinaryFormat/COFF.cpp

+//===- llvm/BinaryFormat/COFF.cpp - The COFF format -----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/BinaryFormat/COFF.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Twine.h"
+
+// 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];
+  }
+}
+
+bool llvm::COFF::encodeSectionName(char *Out, uint64_t Offset) {
+  if (Offset <= Max7DecimalOffset) {
+    // Offsets of 7 digits or less are encoded in ASCII.
+    SmallVector<char, COFF::NameSize> Buffer;
+    Twine('/').concat(Twine(Offset)).toVector(Buffer);
+    assert(Buffer.size() <= COFF::NameSize && Buffer.size() >= 2);
+    std::memcpy(Out, Buffer.data(), Buffer.size());
+    return true;
+  }
+
+  if (Offset <= MaxBase64Offset) {
+    // Starting with 10,000,000, offsets are encoded as base64.
+    encodeBase64StringEntry(Out, Offset);
+    return true;
+  }
+
+  // The offset is too large to be encoded.
+  return false;
+}

llvm/lib/MC/WinCOFFObjectWriter.cpp

if (Local->Data.StorageClass == COFF::IMAGE_SYM_CLASS_NULL) {
       Local->Data.StorageClass = IsExternal ? COFF::IMAGE_SYM_CLASS_EXTERNAL
                                             : COFF::IMAGE_SYM_CLASS_STATIC;
     }
   }
 
   Sym->MC = &MCSym;
 }
 
-// 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];
-  }
-}
-
 void WinCOFFObjectWriter::SetSectionName(COFFSection &S) {
   if (S.Name.size() <= COFF::NameSize) {
     std::memcpy(S.Header.Name, S.Name.c_str(), S.Name.size());
     return;
   }
 
   uint64_t StringTableEntry = Strings.getOffset(S.Name);
-  if (StringTableEntry <= Max7DecimalOffset) {
-    SmallVector<char, COFF::NameSize> Buffer;
-    Twine('/').concat(Twine(StringTableEntry)).toVector(Buffer);
-    assert(Buffer.size() <= COFF::NameSize && Buffer.size() >= 2);
-    std::memcpy(S.Header.Name, Buffer.data(), Buffer.size());
-    return;
-  }
-  if (StringTableEntry <= MaxBase64Offset) {
-    // Starting with 10,000,000, offsets are encoded as base64.
-    encodeBase64StringEntry(S.Header.Name, StringTableEntry);
-    return;
-  }
+  if (!COFF::encodeSectionName(S.Header.Name, StringTableEntry))
     report_fatal_error("COFF string table is greater than 64 GB.");
 }
 
 void WinCOFFObjectWriter::SetSymbolName(COFFSymbol &S) {
   if (S.Name.size() > COFF::NameSize)
     S.set_name_offset(Strings.getOffset(S.Name));
   else
     std::memcpy(S.Data.Name, S.Name.c_str(), S.Name.size());
 }