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

[mstorsjo]

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.

[hans]

@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]

COFF/Writer.cpp

827

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

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]

Applied Reid's suggestions.

[rnk]

lgtm, thanks!

[mstorsjo]

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

[hans]

In D65018#1596934, @mstorsjo wrote:

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

Thanks! Merged both changes to the branch in r366816.

COFF/Writer.cpp

for (Chunk *c : symtab->getChunks()) {
     auto *sc = dyn_cast<SectionChunk>(c);
     if (sc && !sc->live) {
       if (config->verbose)
         sc->printDiscardedMessage();
       continue;
     }
     StringRef name = c->getSectionName();
     // On MinGW, comdat groups are formed by putting the comdat group name
-    // after the '$' in the section name. Such a section name suffix shouldn't
-    // imply separate alphabetical sorting of those section chunks though.
-    if (config->mingw && sc && sc->isCOMDAT())
+    // after the '$' in the section name. For .eh_frame$<symbol>, that must
+    // still be sorted before the .eh_frame trailer from crtend.o, thus just
+    // strip the section name trailer. For other sections, such as
+    // .tls$$<symbol> (where non-comdat .tls symbols are otherwise stored in
+    // ".tls$"), they must be strictly sorted after .tls. And for the
+    // hypothetical case of comdat .CRT$XCU, we definitely need to keep the
+    // suffix for sorting. Thus, to play it safe, only strip the suffix for
+    // .eh_frame.
+    if (config->mingw && sc && sc->isCOMDAT() && name.startswith(".eh_frame$"))
       name = name.split('$').first;
     PartialSection *pSec = createPartialSection(name,
                                                 c->getOutputCharacteristics());
     pSec->chunks.push_back(c);
   }
 
   fixPartialSectionChars(".rsrc", data | r);
   // Even in non MinGW cases, we might need to link against GNU import

test/COFF/tls_suffix_sorting.s

+# REQUIRES: x86
+
+# RUN: echo -e ".section .tls,\"dw\"\n .byte 0xaa\n .section .tls\$ZZZ,\"dw\"\n .byte 0xff\n .globl _tls_index\n .data\n _tls_index:\n .int 0" > %t.tlssup.s
+# RUN: llvm-mc -triple=x86_64-windows-gnu %t.tlssup.s -filetype=obj -o %t.tlssup.o
+# RUN: llvm-mc -triple=x86_64-windows-gnu %s -filetype=obj -o %t.main.o
+
+# RUN: lld-link -lldmingw -entry:main %t.main.o %t.tlssup.o -out:%t.exe
+# RUN: llvm-objdump -s %t.exe | FileCheck %s
+
+# Check that .tls$$foo is sorted after the start marker (aa) and before the
+# end marker (ff).
+
+# CHECK: Contents of section .tls:
+# CHECK:  140004000 aabbff
+
+        .text
+        .globl          main
+main:
+        movl            _tls_index(%rip), %eax
+        movq            %gs:88, %rcx
+        movq            (%rcx,%rax,8), %rax
+        movb            foo@SECREL32(%rax), %al
+        ret
+
+        .section        .tls$$foo,"dw"
+        .linkonce       discard
+foo:
+        .byte           0xbb