[PATCH] D89921: [LLD] [COFF] Align all debug directories

[Petr Penzin via Phabricator via llvm-commits]

penzn added inline comments.


================
Comment at: lld/COFF/Writer.cpp:172
 
 class ExtendedDllCharacteristicsChunk : public NonSectionChunk {
 public:
----------------
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`.


================
Comment at: lld/COFF/Writer.cpp:960
     // 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});
----------------
This can be aligned to match the output of link.exe


================
Comment at: lld/COFF/Writer.cpp:967
         make<ExtendedDllCharacteristicsChunk>(
             IMAGE_DLL_CHARACTERISTICS_EX_CET_COMPAT);
     debugRecords.push_back(
----------------
This needs to be aligned to fix the bug.


================
Comment at: lld/COFF/Writer.cpp:951
   if (config->debug || config->repro || config->cetCompat) {
     debugDirectory = make<DebugDirectoryChunk>(debugRecords, config->repro);
     debugInfoSec->addChunk(debugDirectory);
----------------
rnk wrote:
> penzn wrote:
> > penzn wrote:
> > > rnk wrote:
> > > > Can we make the DebugDirectoryChunk set the default alignment to 4 instead? I believe other chunks do that as well.
> > > Let me try that.
> > 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.
> 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
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89921/new/

https://reviews.llvm.org/D89921