[LLD] [COFF] Fix handling of comdat .drectve sections

[mstorsjo]

This can happen when manually emitting strings into .drectve sections with __attribute__((section(".drectve"))), which is a way to emulate #pragma comment(linker, "...") for mingw compilers, without requiring building with -fms-extensions.

Normally, this doesn't generate any comdat, but if compiled with -fsanitize=address, this section does get turned into a comdat section.

This fixes #67261. This issue can be seen as a regression; a change in the "lli" tool in 17.x triggers this case, if compiled with ASAN enabled, triggering this unsupported corner case in LLD. With this change, LLD can handle it.

[llvmbot]

@llvm/pr-subscribers-lld
@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-lld-coff

Changes

This can happen when manually emitting strings into .drectve sections with __attribute__((section(".drectve"))), which is a way to emulate #pragma comment(linker, "...") for mingw compilers, without requiring building with -fms-extensions.

Normally, this doesn't generate any comdat, but if compiled with -fsanitize=address, this section does get turned into a comdat section.

This fixes #67261. This issue can be seen as a regression; a change in the "lli" tool in 17.x triggers this case, if compiled with ASAN enabled, triggering this unsupported corner case in LLD. With this change, LLD can handle it.

---

Full diff: https://github.com/llvm/llvm-project/pull/68116.diff

2 Files Affected:

• (modified) lld/COFF/InputFiles.cpp (+9-4)
• (added) lld/test/COFF/comdat-drectve.s (+31)

diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 541837a7fceca72..0b0fa4ae3bab868 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -660,10 +660,15 @@ std::optional<Symbol *> ObjFile::createDefined(
 
     if (prevailing) {
       SectionChunk *c = readSection(sectionNumber, def, getName());
-      sparseChunks[sectionNumber] = c;
-      c->sym = cast<DefinedRegular>(leader);
-      c->selection = selection;
-      cast<DefinedRegular>(leader)->data = &c->repl;
+      if (c) {
+        sparseChunks[sectionNumber] = c;
+        c->sym = cast<DefinedRegular>(leader);
+        c->selection = selection;
+        cast<DefinedRegular>(leader)->data = &c->repl;
+      } else {
+        sparseChunks[sectionNumber] = nullptr;
+        return nullptr;
+      }
     } else {
       sparseChunks[sectionNumber] = nullptr;
     }
diff --git a/lld/test/COFF/comdat-drectve.s b/lld/test/COFF/comdat-drectve.s
new file mode 100644
index 000000000000000..6f96a8709fc7703
--- /dev/null
+++ b/lld/test/COFF/comdat-drectve.s
@@ -0,0 +1,31 @@
+# REQUIRES: x86
+
+# RUN: llvm-mc -triple=x86_64-windows-gnu %s -filetype=obj -o %t.obj
+
+# RUN: lld-link %t.obj -out:%t.exe -debug:symtab -subsystem:console
+# RUN: llvm-readobj --coff-exports %t.exe | FileCheck %s
+
+# CHECK: Name: exportedFunc
+
+## This assembly snippet has been reduced from what Clang generates from
+## this C snippet, with -fsanitize=address. Normally, the .drectve
+## section would be a regular section - but when compiled with
+## -fsanitize=address, it becomes a comdat section.
+##
+# void exportedFunc(void) {}
+# void mainCRTStartup(void) {}
+# static __attribute__((section(".drectve"), used)) const char export_chkstk[] =
+#     "-export:exportedFunc";
+
+	.text
+	.globl	exportedFunc
+exportedFunc:
+	retq
+
+	.globl	mainCRTStartup
+mainCRTStartup:
+	retq
+
+	.section	.drectve,"dr",one_only,export_chkstk
+export_chkstk:
+	.asciz	"-export:exportedFunc"

[aganea]

Should we leave that many spaces between .sectionand .drectve, or is this a tab character I don't see?

[mstorsjo]

It's tabs; the whitespace in the test file is exactly as the compiler output it - I've just stripped out parts of the compiler output but not rewritten the whitespace and all that.

[aganea]

nit: you could also insert at L664:

      if (!c)
        return nullptr;

[mstorsjo]

Thanks, I guess that'd work too.

[mstorsjo]

Updated it that way; thanks, that looks even neater!

[rnk]

Seems reasonable.

I take it that we already parse all .drective sections and interpret them as flags, even if they are comdat? Certainly we have to parse the flags before we do comdat resolution.

[rnk]

I see three assignments to sparseChunks[sectionNumber], can we reduce nesting and arrange the code so that we store a variable which is sometimes null?

For your code change, essentially wrap the c-> expressions in the if (c) check.

Is the return nullptr necessary, or can we return leader?

[mstorsjo]

I guess @aganea's suggestion of just moving the if (!c) return nullptr; after assigning sparseChunks[sectionNumber] = c; would simplify it a bit as well.

We can't return leader (I tested that and spent an awful lot of time hunting down why things were screwed up); the cast<DefinedRegular>(leader)->data = &c->repl; assignment is essential - otherwise the leader symbol is left with a nullptr data, which is expected to be initialized before the end.

As we're returning differently (leader vs nullptr) in the two cases where we're assigning nullptr into sparseChunks[sectionNumber], I don't think we very neatly can fold all the three cases together, but @aganea's suggestion might make it more palatable.

[aganea]

I debugged this code too, a while bit ago for https://reviews.llvm.org/D157136 and let's say, the way data flows isn't simple (even if the input data format is simple). Lots of interactions until things converge into place. I always wondered if instead of imperative code for a linker, if a data graph would be easier to follow.

[mstorsjo]

I take it that we already parse all .drective sections and interpret them as flags, even if they are comdat? Certainly we have to parse the flags before we do comdat resolution.

In this case, the contents of the comdat section does get parsed within readSection: https://github.com/llvm/llvm-project/blob/llvmorg-17.0.2/lld/COFF/InputFiles.cpp#L212-L217 (Apparently this stores it within one single StringRef directives; - this wouldn't work right if the file had several separate directive sections.)

In particular, the testcase I'm adding here does test and make sure that the directives in the comdat do get applied.

[aganea]

LGTM, thanks @mstorsjo !