Be explicit about Windows coff name trailing character policy

[serge-sans-paille]

Summary

It's okay to *not* copy the trailing zero of a windows section/symbol name.
This is compatible with strncpy behavior but gcc doesn't know that and
throws an invalid warning. Encode this behavior in a proper function.

Ref: https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#section-table-section-headers
and https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#symbol-name-representation

[rnk]

Why does this suppress the warning? Just the templatization?

llvm/lib/Object/WindowsResource.cpp

520–521

Can we drop the template parameter and make Dest char (&Dest)[COFF::NameSize]? Then all we need is the assert for Src.size() <= COFF::NameSize.

[serge-sans-paille]

The gcc analysis is not interprocedural.

llvm/lib/Object/WindowsResource.cpp

520–521

Sure.

[serge-sans-paille]

Take into account @rnk's review.

[rnk]

lgtm

I guess we're suppressing the warning by rinsing the string literals through StringRef, so GCC can't see the source string size.

[serge-sans-paille]

I guess we're suppressing the warning by rinsing the string literals through StringRef, so GCC can't see the source string size.

Indeed. Thanks for the review!

llvm/lib/Object/WindowsResource.cpp

WindowsResourceCOFFWriter::write(uint32_t TimeDateStamp) {
   writeFirstSection();
   writeSecondSection();
   writeSymbolTable();
   writeStringTable();
 
   return std::move(OutputBuffer);
 }
 
+// According to COFF specification, if the Src has a size equal to Dest,
+// it's okay to *not* copy the trailing zero.
+static void coffnamecpy(char (&Dest)[COFF::NameSize], StringRef Src) {
+  assert(Src.size() <= COFF::NameSize &&
+         "Src is not larger than COFF::NameSize");
+  strncpy(Dest, Src.data(), (size_t)COFF::NameSize);
+}
+
 void WindowsResourceCOFFWriter::writeCOFFHeader(uint32_t TimeDateStamp) {
   // Write the COFF header.
   auto *Header = reinterpret_cast<coff_file_header *>(BufferStart);
   Header->Machine = MachineType;
   Header->NumberOfSections = 2;
   Header->TimeDateStamp = TimeDateStamp;
   Header->PointerToSymbolTable = SymbolTableOffset;
   // One symbol for every resource plus 2 for each section and 1 for @feat.00
   Header->NumberOfSymbols = Data.size() + 5;
   Header->SizeOfOptionalHeader = 0;
   // cvtres.exe sets 32BIT_MACHINE even for 64-bit machine types. Match it.
   Header->Characteristics = COFF::IMAGE_FILE_32BIT_MACHINE;
 }
 
 void WindowsResourceCOFFWriter::writeFirstSectionHeader() {
   // Write the first section header.
   CurrentOffset += sizeof(coff_file_header);
   auto *SectionOneHeader =
       reinterpret_cast<coff_section *>(BufferStart + CurrentOffset);
-  strncpy(SectionOneHeader->Name, ".rsrc$01", (size_t)COFF::NameSize);
+  coffnamecpy(SectionOneHeader->Name, ".rsrc$01");
   SectionOneHeader->VirtualSize = 0;
   SectionOneHeader->VirtualAddress = 0;
   SectionOneHeader->SizeOfRawData = SectionOneSize;
   SectionOneHeader->PointerToRawData = SectionOneOffset;
   SectionOneHeader->PointerToRelocations = SectionOneRelocations;
   SectionOneHeader->PointerToLinenumbers = 0;
   SectionOneHeader->NumberOfRelocations = Data.size();
   SectionOneHeader->NumberOfLinenumbers = 0;
   SectionOneHeader->Characteristics += COFF::IMAGE_SCN_CNT_INITIALIZED_DATA;
   SectionOneHeader->Characteristics += COFF::IMAGE_SCN_MEM_READ;
 }
 
 void WindowsResourceCOFFWriter::writeSecondSectionHeader() {
   // Write the second section header.
   CurrentOffset += sizeof(coff_section);
   auto *SectionTwoHeader =
       reinterpret_cast<coff_section *>(BufferStart + CurrentOffset);
-  strncpy(SectionTwoHeader->Name, ".rsrc$02", (size_t)COFF::NameSize);
+  coffnamecpy(SectionTwoHeader->Name, ".rsrc$02");
   SectionTwoHeader->VirtualSize = 0;
   SectionTwoHeader->VirtualAddress = 0;
   SectionTwoHeader->SizeOfRawData = SectionTwoSize;
   SectionTwoHeader->PointerToRawData = SectionTwoOffset;
   SectionTwoHeader->PointerToRelocations = 0;
   SectionTwoHeader->PointerToLinenumbers = 0;
   SectionTwoHeader->NumberOfRelocations = 0;
   SectionTwoHeader->NumberOfLinenumbers = 0;
void WindowsResourceCOFFWriter::writeSecondSection() {
 
   CurrentOffset = alignTo(CurrentOffset, SECTION_ALIGNMENT);
 }
 
 void WindowsResourceCOFFWriter::writeSymbolTable() {
   // Now write the symbol table.
   // First, the feat symbol.
   auto *Symbol = reinterpret_cast<coff_symbol16 *>(BufferStart + CurrentOffset);
-  strncpy(Symbol->Name.ShortName, "@feat.00", (size_t)COFF::NameSize);
+  coffnamecpy(Symbol->Name.ShortName, "@feat.00");
   Symbol->Value = 0x11;
   Symbol->SectionNumber = 0xffff;
   Symbol->Type = COFF::IMAGE_SYM_DTYPE_NULL;
   Symbol->StorageClass = COFF::IMAGE_SYM_CLASS_STATIC;
   Symbol->NumberOfAuxSymbols = 0;
   CurrentOffset += sizeof(coff_symbol16);
 
   // Now write the .rsrc1 symbol + aux.
   Symbol = reinterpret_cast<coff_symbol16 *>(BufferStart + CurrentOffset);
-  strncpy(Symbol->Name.ShortName, ".rsrc$01", (size_t)COFF::NameSize);
+  coffnamecpy(Symbol->Name.ShortName, ".rsrc$01");
   Symbol->Value = 0;
   Symbol->SectionNumber = 1;
   Symbol->Type = COFF::IMAGE_SYM_DTYPE_NULL;
   Symbol->StorageClass = COFF::IMAGE_SYM_CLASS_STATIC;
   Symbol->NumberOfAuxSymbols = 1;
   CurrentOffset += sizeof(coff_symbol16);
   auto *Aux = reinterpret_cast<coff_aux_section_definition *>(BufferStart +
                                                               CurrentOffset);
   Aux->Length = SectionOneSize;
   Aux->NumberOfRelocations = Data.size();
   Aux->NumberOfLinenumbers = 0;
   Aux->CheckSum = 0;
   Aux->NumberLowPart = 0;
   Aux->Selection = 0;
   CurrentOffset += sizeof(coff_aux_section_definition);
 
   // Now write the .rsrc2 symbol + aux.
   Symbol = reinterpret_cast<coff_symbol16 *>(BufferStart + CurrentOffset);
-  strncpy(Symbol->Name.ShortName, ".rsrc$02", (size_t)COFF::NameSize);
+  coffnamecpy(Symbol->Name.ShortName, ".rsrc$02");
   Symbol->Value = 0;
   Symbol->SectionNumber = 2;
   Symbol->Type = COFF::IMAGE_SYM_DTYPE_NULL;
   Symbol->StorageClass = COFF::IMAGE_SYM_CLASS_STATIC;
   Symbol->NumberOfAuxSymbols = 1;
   CurrentOffset += sizeof(coff_symbol16);
   Aux = reinterpret_cast<coff_aux_section_definition *>(BufferStart +
                                                         CurrentOffset);
   Aux->Length = SectionTwoSize;
   Aux->NumberOfRelocations = 0;
   Aux->NumberOfLinenumbers = 0;
   Aux->CheckSum = 0;
   Aux->NumberLowPart = 0;
   Aux->Selection = 0;
   CurrentOffset += sizeof(coff_aux_section_definition);
 
   // Now write a symbol for each relocation.
   for (unsigned i = 0; i < Data.size(); i++) {
     auto RelocationName = formatv("$R{0:X-6}", i & 0xffffff).sstr<COFF::NameSize>();
     Symbol = reinterpret_cast<coff_symbol16 *>(BufferStart + CurrentOffset);
-    memcpy(Symbol->Name.ShortName, RelocationName.data(), (size_t) COFF::NameSize);
+    coffnamecpy(Symbol->Name.ShortName, RelocationName);
     Symbol->Value = DataOffsets[i];
     Symbol->SectionNumber = 2;
     Symbol->Type = COFF::IMAGE_SYM_DTYPE_NULL;
     Symbol->StorageClass = COFF::IMAGE_SYM_CLASS_STATIC;
     Symbol->NumberOfAuxSymbols = 0;
     CurrentOffset += sizeof(coff_symbol16);
   }
 }