Fix alignment bug in COFF emission.

[ecbeckmann]

Summary

Fix alignment issue in D34020, by aligning all sections to 8 bytes.

[ruiu]

llvm/lib/Object/WindowsResource.cpp

511 ↗

(On Diff #102098)

Do you have to memorize how large was a padding? I mean you may be able to do this.

Current = alignTo(Current, 8)

[ecbeckmann]

llvm/lib/Object/WindowsResource.cpp

511 ↗

(On Diff #102098)

Hmm doesn't that depend on the way the loader aligns cvtres in memory? For example, it could be possible for the system to align the FileOutputBuffer on 4 bytes instead of 8 bytes. So aligning to 8 bytes would actually be aligning to 4 bytes.

[zturner]

llvm/lib/Object/WindowsResource.cpp

511 ↗

(On Diff #102098)

Not sure I follow your comment here. If the system aligns the FileOutputBuffer to 4 bytes (let's be explicit and say it puts it at address 4), and you call Current = alignTo(Current, 8) then after that call Current will be equal to 8, which is what you want, isn't it?

Seems to me like calling

X = OffsetToAlignment(Number, Align)
Number += X;

is mathematically equivalent to:

Number = alignTo(Number, Align);

[ecbeckmann]

llvm/lib/Object/WindowsResource.cpp

511 ↗

(On Diff #102098)

No....this won't be what we want to do.

The end goal is to have the final COFF file be aligned on 8 bytes. Therefore, we want to align with 8 bytes relative to the FileOutputBuffer base address, without regard to the buffer starting alignment. If the address of the buffer starts at 4 and then we say
Current = alignTo(Current, 8), then yes Current will have an absolute aligned-8 address relative to llvm-cvtres memory. However, relative to the COFF file itself it will have alignment 4.

[zturner]

So you're aligning the offset, not the absolute address. In that it might be more intuitive to isntead of storing Current, which is actually a pointer, to store Base and Offset separately, and only operate on (and align) Offset, then compute Base+Offset every time you want to write?

[ecbeckmann]

Keep a running offset from base buffer address rather than moving pointer.

[ecbeckmann]

In D34072#778427, @zturner wrote:

So you're aligning the offset, not the absolute address. In that it might be more intuitive to isntead of storing Current, which is actually a pointer, to store Base and Offset separately, and only operate on (and align) Offset, then compute Base+Offset every time you want to write?

Okay, this probably makes the most sense.

[zturner]

I think you might have gotten this patch combined with the other patch, because now there are a lot of unrelated changes showing up. regardless, lgtm with the new change, but please separate them out again before committing.

[ecbeckmann]

The last diff incorrectly grouped this patch together with the testing infrastructure patch (D34047)

[ruiu]

llvm/trunk/lib/Object/WindowsResource.cpp

35

static?

The following two constants are not ALL_IN_UPPERCAPS, but this is. Since it is not a CPP macro or something, this should be SectionAlignment.

Also, why sizeof(uint64_t) instead of just 8?

469

It is better to increment CurrentOffset in a function that writes something rather than incrementing it in a function that is called after you write it. So move this to writeCOFFHeader.

601–603

write32le(BufferStart + CurrentOffset, 0);

llvm/trunk/lib/Object/WindowsResource.cpp

 namespace object {
 
 #define RETURN_IF_ERROR(X)                                                     \
   if (auto EC = X)                                                             \
     return EC;
 
 const uint32_t MIN_HEADER_SIZE = 7 * sizeof(uint32_t) + 2 * sizeof(uint16_t);
 
+// COFF files seem to be inconsistent with alignment between sections, just use
+// 8-byte because it makes everyone happy.
+const uint32_t SECTION_ALIGNMENT = sizeof(uint64_t);
+
 static const size_t ResourceMagicSize = 16;
 
 static const size_t NullEntrySize = 16;
 
 uint32_t WindowsResourceParser::TreeNode::StringCount = 0;
 uint32_t WindowsResourceParser::TreeNode::DataCount = 0;
 
 WindowsResource::WindowsResource(MemoryBufferRef Source)
private:
   void writeFirstSection();
   void writeSecondSection();
   void writeSymbolTable();
   void writeStringTable();
   void writeDirectoryTree();
   void writeDirectoryStringTable();
   void writeFirstSectionRelocations();
   std::unique_ptr<FileOutputBuffer> Buffer;
-  uint8_t *Current;
+  uint8_t *BufferStart;
+  uint64_t CurrentOffset = 0;
   Machine MachineType;
   const WindowsResourceParser::TreeNode &Resources;
   const ArrayRef<std::vector<uint8_t>> Data;
   uint64_t FileSize;
   uint32_t SymbolTableOffset;
   uint32_t SectionOneSize;
   uint32_t SectionOneOffset;
   uint32_t SectionOneRelocations;
void WindowsResourceCOFFWriter::performSectionOneLayout() {
   }
   SectionOneSize += alignTo(TotalStringTableSize, sizeof(uint32_t));
 
   // account for the relocations of section one.
   SectionOneRelocations = FileSize + SectionOneSize;
   FileSize += SectionOneSize;
   FileSize += Data.size() *
               llvm::COFF::RelocationSize; // one relocation for each resource.
+  FileSize = alignTo(FileSize, SECTION_ALIGNMENT);
 }
 
 void WindowsResourceCOFFWriter::performSectionTwoLayout() {
   // add size of .rsrc$2 section, which contains all resource data on 8-byte
   // alignment.
   SectionTwoOffset = FileSize;
   SectionTwoSize = 0;
   for (auto const &Entry : Data) {
     DataOffsets.push_back(SectionTwoSize);
     SectionTwoSize += llvm::alignTo(Entry.size(), sizeof(uint64_t));
   }
   FileSize += SectionTwoSize;
+  FileSize = alignTo(FileSize, SECTION_ALIGNMENT);
 }
 
 static std::time_t getTime() {
   std::time_t Now = time(nullptr);
   if (Now < 0 || !isUInt<32>(Now))
     return UINT32_MAX;
   return Now;
 }
 
 Error WindowsResourceCOFFWriter::write() {
-  Current = Buffer->getBufferStart();
+  BufferStart = Buffer->getBufferStart();
 
   writeCOFFHeader();
   writeFirstSectionHeader();
   writeSecondSectionHeader();
   writeFirstSection();
   writeSecondSection();
   writeSymbolTable();
   writeStringTable();
 
   if (auto EC = Buffer->commit()) {
     return errorCodeToError(EC);
   }
 
   return Error::success();
 }
 
 void WindowsResourceCOFFWriter::writeCOFFHeader() {
   // Write the COFF header.
-  auto *Header = reinterpret_cast<llvm::object::coff_file_header *>(Current);
+  auto *Header =
+      reinterpret_cast<llvm::object::coff_file_header *>(BufferStart);
   switch (MachineType) {
   case Machine::ARM:
     Header->Machine = llvm::COFF::IMAGE_FILE_MACHINE_ARMNT;
     break;
   case Machine::X64:
     Header->Machine = llvm::COFF::IMAGE_FILE_MACHINE_AMD64;
     break;
   case Machine::X86:
     Header->Machine = llvm::COFF::IMAGE_FILE_MACHINE_I386;
     break;
   default:
     Header->Machine = llvm::COFF::IMAGE_FILE_MACHINE_UNKNOWN;
   }
   Header->NumberOfSections = 2;
   Header->TimeDateStamp = getTime();
   Header->PointerToSymbolTable = SymbolTableOffset;
   // One symbol for every resource plus 2 for each section and @feat.00
   Header->NumberOfSymbols = Data.size() + 5;
   Header->SizeOfOptionalHeader = 0;
   Header->Characteristics = llvm::COFF::IMAGE_FILE_32BIT_MACHINE;
 }
 
 void WindowsResourceCOFFWriter::writeFirstSectionHeader() {
   // Write the first section header.
-  Current += sizeof(llvm::object::coff_file_header);
-  auto *SectionOneHeader =
-      reinterpret_cast<llvm::object::coff_section *>(Current);
+  CurrentOffset += sizeof(llvm::object::coff_file_header);
+  auto *SectionOneHeader = reinterpret_cast<llvm::object::coff_section *>(
+      BufferStart + CurrentOffset);
   strncpy(SectionOneHeader->Name, ".rsrc$01", (size_t)llvm::COFF::NameSize);
   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 = llvm::COFF::IMAGE_SCN_ALIGN_1BYTES;
   SectionOneHeader->Characteristics +=
       llvm::COFF::IMAGE_SCN_CNT_INITIALIZED_DATA;
   SectionOneHeader->Characteristics += llvm::COFF::IMAGE_SCN_MEM_DISCARDABLE;
   SectionOneHeader->Characteristics += llvm::COFF::IMAGE_SCN_MEM_READ;
 }
 
 void WindowsResourceCOFFWriter::writeSecondSectionHeader() {
   // Write the second section header.
-  Current += sizeof(llvm::object::coff_section);
-  auto *SectionTwoHeader =
-      reinterpret_cast<llvm::object::coff_section *>(Current);
+  CurrentOffset += sizeof(llvm::object::coff_section);
+  auto *SectionTwoHeader = reinterpret_cast<llvm::object::coff_section *>(
+      BufferStart + CurrentOffset);
   strncpy(SectionTwoHeader->Name, ".rsrc$02", (size_t)llvm::COFF::NameSize);
   SectionTwoHeader->VirtualSize = 0;
   SectionTwoHeader->VirtualAddress = 0;
   SectionTwoHeader->SizeOfRawData = SectionTwoSize;
   SectionTwoHeader->PointerToRawData = SectionTwoOffset;
   SectionTwoHeader->PointerToRelocations = 0;
   SectionTwoHeader->PointerToLinenumbers = 0;
   SectionTwoHeader->NumberOfRelocations = 0;
   SectionTwoHeader->NumberOfLinenumbers = 0;
   SectionTwoHeader->Characteristics =
       llvm::COFF::IMAGE_SCN_CNT_INITIALIZED_DATA;
   SectionTwoHeader->Characteristics += llvm::COFF::IMAGE_SCN_MEM_READ;
 }
 
 void WindowsResourceCOFFWriter::writeFirstSection() {
   // Write section one.
-  Current += sizeof(llvm::object::coff_section);
+  CurrentOffset += sizeof(llvm::object::coff_section);
 
   writeDirectoryTree();
   writeDirectoryStringTable();
   writeFirstSectionRelocations();
+
+  CurrentOffset = alignTo(CurrentOffset, SECTION_ALIGNMENT);
 }
 
 void WindowsResourceCOFFWriter::writeSecondSection() {
   // Now write the .rsrc$02 section.
   for (auto const &RawDataEntry : Data) {
-    std::copy(RawDataEntry.begin(), RawDataEntry.end(), Current);
-    Current += alignTo(RawDataEntry.size(), sizeof(uint64_t));
+    std::copy(RawDataEntry.begin(), RawDataEntry.end(),
+              BufferStart + CurrentOffset);
+    CurrentOffset += alignTo(RawDataEntry.size(), sizeof(uint64_t));
   }
+
+  CurrentOffset = alignTo(CurrentOffset, SECTION_ALIGNMENT);
 }
 
 void WindowsResourceCOFFWriter::writeSymbolTable() {
   // Now write the symbol table.
   // First, the feat symbol.
-  auto *Symbol = reinterpret_cast<llvm::object::coff_symbol16 *>(Current);
+  auto *Symbol = reinterpret_cast<llvm::object::coff_symbol16 *>(BufferStart +
+                                                                 CurrentOffset);
   strncpy(Symbol->Name.ShortName, "@feat.00", (size_t)llvm::COFF::NameSize);
   Symbol->Value = 0x11;
   Symbol->SectionNumber = 0xffff;
   Symbol->Type = llvm::COFF::IMAGE_SYM_DTYPE_NULL;
   Symbol->StorageClass = llvm::COFF::IMAGE_SYM_CLASS_STATIC;
   Symbol->NumberOfAuxSymbols = 0;
-  Current += sizeof(llvm::object::coff_symbol16);
+  CurrentOffset += sizeof(llvm::object::coff_symbol16);
 
   // Now write the .rsrc1 symbol + aux.
-  Symbol = reinterpret_cast<llvm::object::coff_symbol16 *>(Current);
+  Symbol = reinterpret_cast<llvm::object::coff_symbol16 *>(BufferStart +
+                                                           CurrentOffset);
   strncpy(Symbol->Name.ShortName, ".rsrc$01", (size_t)llvm::COFF::NameSize);
   Symbol->Value = 0;
   Symbol->SectionNumber = 1;
   Symbol->Type = llvm::COFF::IMAGE_SYM_DTYPE_NULL;
   Symbol->StorageClass = llvm::COFF::IMAGE_SYM_CLASS_STATIC;
   Symbol->NumberOfAuxSymbols = 1;
-  Current += sizeof(llvm::object::coff_symbol16);
-  auto *Aux =
-      reinterpret_cast<llvm::object::coff_aux_section_definition *>(Current);
+  CurrentOffset += sizeof(llvm::object::coff_symbol16);
+  auto *Aux = reinterpret_cast<llvm::object::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;
-  Current += sizeof(llvm::object::coff_aux_section_definition);
+  CurrentOffset += sizeof(llvm::object::coff_aux_section_definition);
 
   // Now write the .rsrc2 symbol + aux.
-  Symbol = reinterpret_cast<llvm::object::coff_symbol16 *>(Current);
+  Symbol = reinterpret_cast<llvm::object::coff_symbol16 *>(BufferStart +
+                                                           CurrentOffset);
   strncpy(Symbol->Name.ShortName, ".rsrc$02", (size_t)llvm::COFF::NameSize);
   Symbol->Value = 0;
   Symbol->SectionNumber = 2;
   Symbol->Type = llvm::COFF::IMAGE_SYM_DTYPE_NULL;
   Symbol->StorageClass = llvm::COFF::IMAGE_SYM_CLASS_STATIC;
   Symbol->NumberOfAuxSymbols = 1;
-  Current += sizeof(llvm::object::coff_symbol16);
-  Aux = reinterpret_cast<llvm::object::coff_aux_section_definition *>(Current);
+  CurrentOffset += sizeof(llvm::object::coff_symbol16);
+  Aux = reinterpret_cast<llvm::object::coff_aux_section_definition *>(
+      BufferStart + CurrentOffset);
   Aux->Length = SectionTwoSize;
   Aux->NumberOfRelocations = 0;
   Aux->NumberOfLinenumbers = 0;
   Aux->CheckSum = 0;
   Aux->NumberLowPart = 0;
   Aux->Selection = 0;
-  Current += sizeof(llvm::object::coff_aux_section_definition);
+  CurrentOffset += sizeof(llvm::object::coff_aux_section_definition);
 
   // Now write a symbol for each relocation.
   for (unsigned i = 0; i < Data.size(); i++) {
     char RelocationName[9];
     sprintf(RelocationName, "$R%06X", DataOffsets[i]);
-    Symbol = reinterpret_cast<llvm::object::coff_symbol16 *>(Current);
+    Symbol = reinterpret_cast<llvm::object::coff_symbol16 *>(BufferStart +
+                                                             CurrentOffset);
     strncpy(Symbol->Name.ShortName, RelocationName,
             (size_t)llvm::COFF::NameSize);
     Symbol->Value = DataOffsets[i];
     Symbol->SectionNumber = 1;
     Symbol->Type = llvm::COFF::IMAGE_SYM_DTYPE_NULL;
     Symbol->StorageClass = llvm::COFF::IMAGE_SYM_CLASS_STATIC;
     Symbol->NumberOfAuxSymbols = 0;
-    Current += sizeof(llvm::object::coff_symbol16);
+    CurrentOffset += sizeof(llvm::object::coff_symbol16);
   }
 }
 
 void WindowsResourceCOFFWriter::writeStringTable() {
   // Just 4 null bytes for the string table.
-  auto COFFStringTable = reinterpret_cast<void *>(Current);
-  memset(COFFStringTable, 0, 4);
+  auto COFFStringTable =
+      reinterpret_cast<uint32_t *>(BufferStart + CurrentOffset);
+  *COFFStringTable = 0;
 }
 
 void WindowsResourceCOFFWriter::writeDirectoryTree() {
   // Traverse parsed resource tree breadth-first and write the corresponding
   // COFF objects.
   std::queue<const WindowsResourceParser::TreeNode *> Queue;
   Queue.push(&Resources);
   uint32_t NextLevelOffset = sizeof(llvm::object::coff_resource_dir_table) +
                              (Resources.getStringChildren().size() +
                               Resources.getIDChildren().size()) *
                                  sizeof(llvm::object::coff_resource_dir_entry);
   std::vector<const WindowsResourceParser::TreeNode *> DataEntriesTreeOrder;
   uint32_t CurrentRelativeOffset = 0;
 
   while (!Queue.empty()) {
     auto CurrentNode = Queue.front();
     Queue.pop();
-    auto *Table =
-        reinterpret_cast<llvm::object::coff_resource_dir_table *>(Current);
+    auto *Table = reinterpret_cast<llvm::object::coff_resource_dir_table *>(
+        BufferStart + CurrentOffset);
     Table->Characteristics = CurrentNode->getCharacteristics();
     Table->TimeDateStamp = 0;
     Table->MajorVersion = CurrentNode->getMajorVersion();
     Table->MinorVersion = CurrentNode->getMinorVersion();
     auto &IDChildren = CurrentNode->getIDChildren();
     auto &StringChildren = CurrentNode->getStringChildren();
     Table->NumberOfNameEntries = StringChildren.size();
     Table->NumberOfIDEntries = IDChildren.size();
-    Current += sizeof(llvm::object::coff_resource_dir_table);
+    CurrentOffset += sizeof(llvm::object::coff_resource_dir_table);
     CurrentRelativeOffset += sizeof(llvm::object::coff_resource_dir_table);
 
     // Write the directory entries immediately following each directory table.
     for (auto const &Child : StringChildren) {
-      auto *Entry =
-          reinterpret_cast<llvm::object::coff_resource_dir_entry *>(Current);
+      auto *Entry = reinterpret_cast<llvm::object::coff_resource_dir_entry *>(
+          BufferStart + CurrentOffset);
       Entry->Identifier.NameOffset =
           StringTableOffsets[Child.second->getStringIndex()];
       if (Child.second->checkIsDataNode()) {
         Entry->Offset.DataEntryOffset = NextLevelOffset;
         NextLevelOffset += sizeof(llvm::object::coff_resource_data_entry);
         DataEntriesTreeOrder.push_back(Child.second.get());
       } else {
         Entry->Offset.SubdirOffset = NextLevelOffset + (1 << 31);
         NextLevelOffset += sizeof(llvm::object::coff_resource_dir_table) +
                            (Child.second->getStringChildren().size() +
                             Child.second->getIDChildren().size()) *
                                sizeof(llvm::object::coff_resource_dir_entry);
         Queue.push(Child.second.get());
       }
-      Current += sizeof(llvm::object::coff_resource_dir_entry);
+      CurrentOffset += sizeof(llvm::object::coff_resource_dir_entry);
       CurrentRelativeOffset += sizeof(llvm::object::coff_resource_dir_entry);
     }
     for (auto const &Child : IDChildren) {
-      auto *Entry =
-          reinterpret_cast<llvm::object::coff_resource_dir_entry *>(Current);
+      auto *Entry = reinterpret_cast<llvm::object::coff_resource_dir_entry *>(
+          BufferStart + CurrentOffset);
       Entry->Identifier.ID = Child.first;
       if (Child.second->checkIsDataNode()) {
         Entry->Offset.DataEntryOffset = NextLevelOffset;
         NextLevelOffset += sizeof(llvm::object::coff_resource_data_entry);
         DataEntriesTreeOrder.push_back(Child.second.get());
       } else {
         Entry->Offset.SubdirOffset = NextLevelOffset + (1 << 31);
         NextLevelOffset += sizeof(llvm::object::coff_resource_dir_table) +
                            (Child.second->getStringChildren().size() +
                             Child.second->getIDChildren().size()) *
                                sizeof(llvm::object::coff_resource_dir_entry);
         Queue.push(Child.second.get());
       }
-      Current += sizeof(llvm::object::coff_resource_dir_entry);
+      CurrentOffset += sizeof(llvm::object::coff_resource_dir_entry);
       CurrentRelativeOffset += sizeof(llvm::object::coff_resource_dir_entry);
     }
   }
 
   RelocationAddresses.resize(Data.size());
   // Now write all the resource data entries.
   for (auto DataNodes : DataEntriesTreeOrder) {
-    auto *Entry =
-        reinterpret_cast<llvm::object::coff_resource_data_entry *>(Current);
+    auto *Entry = reinterpret_cast<llvm::object::coff_resource_data_entry *>(
+        BufferStart + CurrentOffset);
     RelocationAddresses[DataNodes->getDataIndex()] = CurrentRelativeOffset;
     Entry->DataRVA = 0; // Set to zero because it is a relocation.
     Entry->DataSize = Data[DataNodes->getDataIndex()].size();
     Entry->Codepage = 0;
     Entry->Reserved = 0;
-    Current += sizeof(llvm::object::coff_resource_data_entry);
+    CurrentOffset += sizeof(llvm::object::coff_resource_data_entry);
     CurrentRelativeOffset += sizeof(llvm::object::coff_resource_data_entry);
   }
 }
 
 void WindowsResourceCOFFWriter::writeDirectoryStringTable() {
   // Now write the directory string table for .rsrc$01
   uint32_t TotalStringTableSize = 0;
   for (auto String : StringTable) {
-    auto *LengthField = reinterpret_cast<uint16_t *>(Current);
+    auto *LengthField =
+        reinterpret_cast<uint16_t *>(BufferStart + CurrentOffset);
     uint16_t Length = String.size();
     *LengthField = Length;
-    Current += sizeof(uint16_t);
-    auto *Start = reinterpret_cast<UTF16 *>(Current);
+    CurrentOffset += sizeof(uint16_t);
+    auto *Start = reinterpret_cast<UTF16 *>(BufferStart + CurrentOffset);
     std::copy(String.begin(), String.end(), Start);
-    Current += Length * sizeof(UTF16);
+    CurrentOffset += Length * sizeof(UTF16);
     TotalStringTableSize += Length * sizeof(UTF16) + sizeof(uint16_t);
   }
-  Current +=
+  CurrentOffset +=
       alignTo(TotalStringTableSize, sizeof(uint32_t)) - TotalStringTableSize;
 }
 
 void WindowsResourceCOFFWriter::writeFirstSectionRelocations() {
 
   // Now write the relocations for .rsrc$01
   // Five symbols already in table before we start, @feat.00 and 2 for each
   // .rsrc section.
   uint32_t NextSymbolIndex = 5;
   for (unsigned i = 0; i < Data.size(); i++) {
-    auto *Reloc = reinterpret_cast<llvm::object::coff_relocation *>(Current);
+    auto *Reloc = reinterpret_cast<llvm::object::coff_relocation *>(
+        BufferStart + CurrentOffset);
     Reloc->VirtualAddress = RelocationAddresses[i];
     Reloc->SymbolTableIndex = NextSymbolIndex++;
     switch (MachineType) {
     case Machine::ARM:
       Reloc->Type = llvm::COFF::IMAGE_REL_ARM_ADDR32NB;
       break;
     case Machine::X64:
       Reloc->Type = llvm::COFF::IMAGE_REL_AMD64_ADDR32NB;
       break;
     case Machine::X86:
       Reloc->Type = llvm::COFF::IMAGE_REL_I386_DIR32NB;
       break;
     default:
       Reloc->Type = 0;
     }
-    Current += sizeof(llvm::object::coff_relocation);
+    CurrentOffset += sizeof(llvm::object::coff_relocation);
   }
 }
 
 Error writeWindowsResourceCOFF(StringRef OutputFile, Machine MachineType,
                                const WindowsResourceParser &Parser) {
   Error E = Error::success();
   WindowsResourceCOFFWriter Writer(OutputFile, MachineType, Parser, E);
   if (E)
     return E;
   return Writer.write();
 }
 
 } // namespace object
 } // namespace llvm