[llvm-objcopy] [COFF] Fix writing object files without symbols/string table

[mstorsjo]

Summary

Previously, this was broken - by setting PointerToSymbolTable to zero but still actually writing the string table length, the object file header was corrupted.

Splitting out this independent fix from D56481.

[jhenderson]

I'm not quite sure I follow why this only affects executable files? Why is this an issue in PE but not plain COFF?

[mstorsjo]

In D56584#1353916, @jhenderson wrote:

I'm not quite sure I follow why this only affects executable files? Why is this an issue in PE but not plain COFF?

Initially I tried to make the output of llvm-objcopy as close as possible to the output from various tools (llvm-mc and MSVC), and to achieve this, I only skipped including the symbol/string table in executables, while keeping it present in object files.

I know that exactly matching the output of another tool shouldn't be a goal in itself, but I don't know if there are tools out there that would flat out reject an object file with an omitted symbol/string table, as opposed to an empty symbol/string table. So keeping this distinction probably is safer.

[jhenderson]

In D56584#1353933, @mstorsjo wrote:

In D56584#1353916, @jhenderson wrote:

I'm not quite sure I follow why this only affects executable files? Why is this an issue in PE but not plain COFF?

Initially I tried to make the output of llvm-objcopy as close as possible to the output from various tools (llvm-mc and MSVC), and to achieve this, I only skipped including the symbol/string table in executables, while keeping it present in object files.

I know that exactly matching the output of another tool shouldn't be a goal in itself, but I don't know if there are tools out there that would flat out reject an object file with an omitted symbol/string table, as opposed to an empty symbol/string table. So keeping this distinction probably is safer.

So the behaviour change is that for objects, the object now has a pointer to the symbol table, which it didn't before. I think I get it now. The problem is the writeSymbolStringTables function, and you want that to always write a size for object files, even if the table is empty, right?

[mstorsjo]

In D56584#1353961, @jhenderson wrote:

In D56584#1353933, @mstorsjo wrote:

In D56584#1353916, @jhenderson wrote:

I'm not quite sure I follow why this only affects executable files? Why is this an issue in PE but not plain COFF?

Initially I tried to make the output of llvm-objcopy as close as possible to the output from various tools (llvm-mc and MSVC), and to achieve this, I only skipped including the symbol/string table in executables, while keeping it present in object files.

I know that exactly matching the output of another tool shouldn't be a goal in itself, but I don't know if there are tools out there that would flat out reject an object file with an omitted symbol/string table, as opposed to an empty symbol/string table. So keeping this distinction probably is safer.

So the behaviour change is that for objects, the object now has a pointer to the symbol table, which it didn't before. I think I get it now. The problem is the writeSymbolStringTables function, and you want that to always write a size for object files, even if the table is empty, right?

Correct. Previously the pointer to the symbol table was zero, so the string table length (which follows directly after the symbol table) was written at offset 0 in the file, clobbering the header.

[jhenderson]

LGTM.

test/tools/llvm-objcopy/COFF/Inputs/no-symbols.yaml

+--- !COFF
+header:          
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:        
+  - Name:            .text
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     E800000000C3C3C3
+symbols:         
+...

test/tools/llvm-objcopy/COFF/basic-copy.test

 RUN: obj2yaml %t.i386-copy.exe > %t.i386-copy.exe.yaml
 RUN: cmp %t.i386.exe.yaml %t.i386-copy.exe.yaml
 
 RUN: yaml2obj %p/Inputs/x86_64-exe.yaml > %t.x86_64.exe
 RUN: llvm-objcopy %t.x86_64.exe %t.x86_64-copy.exe
 RUN: obj2yaml %t.x86_64.exe > %t.x86_64.exe.yaml
 RUN: obj2yaml %t.x86_64-copy.exe > %t.x86_64-copy.exe.yaml
 RUN: cmp %t.x86_64.exe.yaml %t.x86_64-copy.exe.yaml
+
+RUN: yaml2obj %p/Inputs/no-symbols.yaml > %t.no-symbols.o
+RUN: llvm-objcopy %t.no-symbols.o %t.no-symbols-copy.o
+RUN: obj2yaml %t.no-symbols.o > %t.no-symbols.o.yaml
+RUN: obj2yaml %t.no-symbols-copy.o > %t.no-symbols-copy.o.yaml
+RUN: cmp %t.no-symbols.o.yaml %t.no-symbols-copy.o.yaml

tools/llvm-objcopy/COFF/Writer.cpp

Error COFFWriter::finalize(bool IsBigObj) {
   size_t SymTabSize, SymbolSize;
   std::tie(SymTabSize, SymbolSize) = IsBigObj
                                          ? finalizeSymbolTable<coff_symbol32>()
                                          : finalizeSymbolTable<coff_symbol16>();
 
   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) {
-    // Don't point to the symbol table if empty.
+  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;
-    // For executables, skip the length field of an empty string table.
-    if (Obj.IsPE)
-      StrTabSize = 0;
   }
 
   size_t NumRawSymbols = SymTabSize / SymbolSize;
   Obj.CoffFileHeader.PointerToSymbolTable = PointerToSymbolTable;
   Obj.CoffFileHeader.NumberOfSymbols = NumRawSymbols;
   FileSize += SymTabSize + StrTabSize;
   FileSize = alignTo(FileSize, FileAlignment);