Page MenuHomePhabricator

[LLD] [COFF] Align all debug directories
ClosedPublic

Authored by rnk on Oct 21 2020, 7:16 PM.

Details

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

Diff Detail

Event Timeline

penzn created this revision.Oct 21 2020, 7:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2020, 7:16 PM
penzn requested review of this revision.Oct 21 2020, 7:16 PM

LGTM

lld/COFF/Writer.cpp
988

now space alignment is broken :)

do we need to update some tests?

vitalybuka edited the summary of this revision. (Show Details)Oct 21 2020, 7:56 PM
vitalybuka edited the summary of this revision. (Show Details)
rnk added a subscriber: rnk.Oct 22 2020, 10:56 AM
rnk added inline comments.
lld/COFF/Writer.cpp
966–967

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

penzn added a comment.Oct 22 2020, 8:05 PM

do we need to update some tests?

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

lld/COFF/Writer.cpp
966–967

Let me try that.

988

Ah, two spaces.

penzn updated this revision to Diff 300155.Oct 22 2020, 8:16 PM

Fix formatting in Writer.cpp.

penzn added a comment.Oct 22 2020, 8:17 PM

Great, looks like I broke the diff.

penzn updated this revision to Diff 300156.Oct 22 2020, 8:23 PM

Fix broken diff.

penzn marked an inline comment as done.Oct 22 2020, 8:31 PM
penzn added inline comments.
lld/COFF/Writer.cpp
966–967

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.

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

rnk added inline comments.Oct 26 2020, 11:44 AM
lld/COFF/Writer.cpp
966–967

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 added inline comments.Oct 27 2020, 5:00 PM
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.

966–967

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.

976

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

980–981

This needs to be aligned to fix the bug.

rnk commandeered this revision.Oct 29 2020, 3:48 PM
rnk edited reviewers, added: penzn; removed: 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 updated this revision to Diff 301779.Oct 29 2020, 3:52 PM
  • simplified version
penzn added a comment.Oct 30 2020, 4:31 PM

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

lld/COFF/Writer.cpp
988

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 added inline comments.Nov 2 2020, 10:50 AM
lld/COFF/Writer.cpp
988

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.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 2 2020, 10:51 AM
Closed by commit rGe59726220f3d: [LLD] [COFF] Align all debug directories (authored by penzn, committed by rnk). · Explain Why
This revision was automatically updated to reflect the committed changes.