Page MenuHomePhabricator

[LLD] [COFF] Unbreak sorting of mingw comdat .tls sections after SVN r363457
ClosedPublic

Authored by mstorsjo on Jul 19 2019, 2:04 PM.

Details

Summary

Code built for mingw with -fdata-sections will store each TLS variable in a comdat section, named .tls$$<varname>. Normal TLS variables are stored in sections named .tls$ with a trailing dollar, which are sorted before a starter marker (in a later linked object file) in a section named ".tls" (with no dollar suffix), before an ending marker in a section named ".tls$ZZZ".

The mingw comdat section suffix stripping introduced in SVN r363457 broke sorting of such tls sections, ending up sorting the stripped .tls$$<varname> sections (stripped to ".tls") before the start marker in the section named ".tls".

We could either add exceptions to the section name suffix stripping for .tls (and .CRT, where suffixes always should be honored), but the more conservative option is probably the reverse; to only apply the stripping for the normal sections where sorting shouldn't have any effect.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Jul 19 2019, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2019, 2:05 PM
hans added a comment.Jul 22 2019, 1:29 PM

@hans This is a fix for a regression from June, that needs to go to the 9 branch once we settle upon how to handle it.

Thanks, I'll keep a look on it.

rnk added inline comments.Jul 22 2019, 1:44 PM
COFF/Writer.cpp
827 ↗(On Diff #210892)

To improve the readability, I'd make the condition a helper. I suppose it needs sc and name. You can elaborate on the whitelist/blacklist reasoning there.

833 ↗(On Diff #210892)

I think I would apply the same logic to the standard sections of .text, .data, .rdata, .pdata, and .xdata. Otherwise we'll make three unneeded PartialSections for every inline function on x64 and do more sorting than necessary.

mstorsjo updated this revision to Diff 211188.Jul 22 2019, 2:13 PM
mstorsjo edited the summary of this revision. (Show Details)

Applied Reid's suggestions.

rnk accepted this revision.Jul 22 2019, 3:11 PM

lgtm, thanks!

This revision is now accepted and ready to land.Jul 22 2019, 3:11 PM
This revision was automatically updated to reflect the committed changes.

@hans FYI, this needed a followup fix for tests on windows buildbots, in rL366784.

hans added a comment.Jul 23 2019, 8:22 AM

@hans FYI, this needed a followup fix for tests on windows buildbots, in rL366784.

Thanks! Merged both changes to the branch in r366816.