[LLD] [COFF] Align all debug directories

[rnk]

Summary

Match MSVC linker output - align all debug directories on four bytes, while removing debug directory alignment. This would have the same effect on CETCOMPAT support as D89919.

Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1136664

[vitalybuka]

LGTM

lld/COFF/Writer.cpp

976

now space alignment is broken :)

[vitalybuka]

do we need to update some tests?

[rnk]

lld/COFF/Writer.cpp

951

Can we make the DebugDirectoryChunk set the default alignment to 4 instead? I believe other chunks do that as well.

[penzn]

In D89921#2346184, @vitalybuka wrote:

do we need to update some tests?

Do you mean the tests that failed? I can't reproduce them yet.

lld/COFF/Writer.cpp

951

Let me try that.

976

Ah, two spaces.

[penzn]

Fix formatting in Writer.cpp.

[penzn]

Great, looks like I broke the diff.

[penzn]

Fix broken diff.

[penzn]

lld/COFF/Writer.cpp

951

On the other hand, do you mean NonSectionChunk? DebugDirectoryChunk and the debug directory sections (CV, ExtendedDllChars) all derive from it. Also, the alignment here was added in D70606.

[vitalybuka]

Reid, can you please continue the review? Looks like Rui didn't review patches recently.

[rnk]

lld/COFF/Writer.cpp

951

Sorry, I meant the constructor of DebugDirectoryChunk should set the alignment to 4. IMO that's simpler than setting it in the loop below, and ensures all debug directories are 4 byte aligned.

Other chunks set their default alignment like this:
https://github.com/llvm/llvm-project/blob/master/lld/COFF/Chunks.h#L491

[penzn]

lld/COFF/Writer.cpp

172

C++ class for the chunk we must align - it derives from NonSectionChunk. Other chunks on the same level (CodeView for example) are also deriving from NonSectionChunk.

951

There is a bit of confusing terminology here. When dumpbin says that "the following debug directories" it refers not to this chunk, but to its contents (what is stored in debugRecords list). We can still align DebugDirectoryChunk in spirit of matching link.exe output, thought it does not seem to have effect on the outcome with this bug.

The chunk we have to align is ExtendedDllCharacteristicsChunk and in this change we are also looking to align other chunks at the same level. All of them derive from NonSectionChunk, so setting default alignment for DebugDirectoryChunk would not really help.

I've added some comments to point to definition and instantiations.

It is possible to take the default alignment route, but that would require adding that to a handful of classes, rather than just setting it in this function.

960

This can be aligned to match the output of link.exe

967

This needs to be aligned to fix the bug.

[rnk]

I see, sorry for the misunderstanding. Let me upload a version of this with some more simplifications, tell me if it looks correct.

[rnk]

• simplified version

[penzn]

I think it works. We can keep "less than 4" check or keep it as is.

lld/COFF/Writer.cpp

979

Do we need "less than four" check? To be honest, I've lifted that from createGuardCFTable, and either way wold work for the issue we are fixing. I don't think /align flag would affect the alignment on this level.

[rnk]

lld/COFF/Writer.cpp

979

I don't think so. So far there are at most two kinds of sections here, just the two added above. They both need 4 byte alignment, and can't receive a higher alignment from anywhere else. We need to be careful in the cfguard code because we are adjusting the alignment of user-provided sections, so they could've requested a 4k alignment, for example, and we wouldn't want to reduce it.

lld/COFF/Writer.cpp

void writeTo(uint8_t *b) const override {
     if (!config->pdbAltPath.empty())
       memcpy(p, config->pdbAltPath.data(), config->pdbAltPath.size());
     p[config->pdbAltPath.size()] = '\0';
   }
 
   mutable codeview::DebugInfo *buildId = nullptr;
 };
 
 class ExtendedDllCharacteristicsChunk : public NonSectionChunk {
 public:
   ExtendedDllCharacteristicsChunk(uint32_t c) : characteristics(c) {}
 
   size_t getSize() const override { return 4; }
 
   void writeTo(uint8_t *buf) const override { write32le(buf, characteristics); }
 
   uint32_t characteristics = 0;
void Writer::createMiscChunks() {
   if (!symtab->localImportChunks.empty()) {
     for (Chunk *c : symtab->localImportChunks)
       rdataSec->addChunk(c);
   }
 
   // Create Debug Information Chunks
   OutputSection *debugInfoSec = config->mingw ? buildidSec : rdataSec;
   if (config->debug || config->repro || config->cetCompat) {
     debugDirectory = make<DebugDirectoryChunk>(debugRecords, config->repro);
-    debugDirectory->setAlignment(4);
     debugInfoSec->addChunk(debugDirectory);
   }
 
   if (config->debug) {
     // Make a CVDebugRecordChunk even when /DEBUG:CV is not specified.  We
     // output a PDB no matter what, and this chunk provides the only means of
     // allowing a debugger to match a PDB and an executable.  So we need it even
     // if we're ultimately not going to write CodeView data to the PDB.
     buildId = make<CVDebugRecordChunk>();
     debugRecords.push_back({COFF::IMAGE_DEBUG_TYPE_CODEVIEW, buildId});
   }
 
   if (config->cetCompat) {
     ExtendedDllCharacteristicsChunk *extendedDllChars =
         make<ExtendedDllCharacteristicsChunk>(
             IMAGE_DLL_CHARACTERISTICS_EX_CET_COMPAT);
     debugRecords.push_back(
         {COFF::IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS, extendedDllChars});
   }
 
   if (debugRecords.size() > 0) {
-    for (std::pair<COFF::DebugType, Chunk *> r : debugRecords)
+    for (std::pair<COFF::DebugType, Chunk *> r : debugRecords) {
+      // Ensure that every debug directory is aligned on 4 bytes
+      if (r.second->getAlignment() < 4) 
+          r.second->setAlignment(4);
       debugInfoSec->addChunk(r.second);
     }
+  }
 
   // Create SEH table. x86-only.
   if (config->safeSEH)
     createSEHTable();
 
   // Create /guard:cf tables if requested.
   if (config->guardCF != GuardCFLevel::Off)
     createGuardCFTables();
 
   if (config->autoImport)
     createRuntimePseudoRelocs();
 
   if (config->mingw)
     insertCtorDtorSymbols();
 }
 
 // Create .idata section for the DLL-imported symbol table.
 // The format of this section is inherently Windows-specific.
 // IdataContents class abstracted away the details for us,