[LLD][COFF] Fix importing DllMain from import libraries #146610
Conversation
|
@llvm/pr-subscribers-lld-coff @llvm/pr-subscribers-lld Author: Alexandre Ganea (aganea) ChangesThis is a workaround for #82050 by skipping importing the Full diff: https://github.com/llvm/llvm-project/pull/146610.diff 4 Files Affected:
diff --git a/lld/COFF/Config.h b/lld/COFF/Config.h
index 473943a239422..79b63e5b7236f 100644
--- a/lld/COFF/Config.h
+++ b/lld/COFF/Config.h
@@ -307,6 +307,7 @@ struct Configuration {
bool warnDebugInfoUnusable = true;
bool warnLongSectionNames = true;
bool warnStdcallFixup = true;
+ bool warnExportedDllMain = true;
bool incremental = true;
bool integrityCheck = false;
bool killAt = false;
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 8a016d44d4636..283aeed1a19cd 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -1643,6 +1643,8 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
config->warnLocallyDefinedImported = false;
else if (s == "longsections")
config->warnLongSectionNames = false;
+ else if (s == "exporteddllmain")
+ config->warnExportedDllMain = false;
// Other warning numbers are ignored.
}
}
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 760ce00dda0d6..0b7dbea8cdd99 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -114,6 +114,35 @@ static coff_symbol_generic *cloneSymbol(COFFSymbolRef sym) {
}
}
+// Skip importing DllMain thunks from import libraries.
+static bool fixupDllMain(COFFLinkerContext &ctx, llvm::object::Archive *file,
+ const Archive::Symbol &sym, bool &skipDllMain) {
+ if (skipDllMain)
+ return true;
+ const Archive::Child &c =
+ CHECK(sym.getMember(), file->getFileName() +
+ ": could not get the member for symbol " +
+ toCOFFString(ctx, sym));
+ MemoryBufferRef mb =
+ CHECK(c.getMemoryBufferRef(),
+ file->getFileName() +
+ ": could not get the buffer for a child buffer of the archive");
+ if (identify_magic(mb.getBuffer()) == file_magic::coff_import_library) {
+ if (ctx.config.warnExportedDllMain) {
+ // We won't place DllMain symbols in the symbol table if they are
+ // coming from a import library. This message can be ignored with the flag
+ // '/ignore:exporteddllmain'
+ Warn(ctx)
+ << file->getFileName()
+ << ": skipping exported DllMain symbol [exporteddllmain]\nNOTE: this "
+ "might be a mistake when the DLL/library was produced.";
+ }
+ skipDllMain = true;
+ return true;
+ }
+ return false;
+}
+
ArchiveFile::ArchiveFile(COFFLinkerContext &ctx, MemoryBufferRef m)
: InputFile(ctx.symtab, ArchiveKind, m) {}
@@ -176,8 +205,17 @@ void ArchiveFile::parse() {
}
// Read the symbol table to construct Lazy objects.
- for (const Archive::Symbol &sym : file->symbols())
+ bool skipDllMain = false;
+ for (const Archive::Symbol &sym : file->symbols()) {
+ // If the DllMain symbol was exported by mistake, skip importing it
+ // otherwise we might end up with a import thunk in the final binary which
+ // is wrong.
+ if (sym.getName() == "__imp_DllMain" || sym.getName() == "DllMain") {
+ if (fixupDllMain(ctx, file.get(), sym, skipDllMain))
+ continue;
+ }
archiveSymtab->addLazyArchive(this, sym);
+ }
}
// Returns a buffer pointing to a member file containing a given symbol.
diff --git a/lld/test/COFF/exported-dllmain.test b/lld/test/COFF/exported-dllmain.test
new file mode 100644
index 0000000000000..fcf6ed1005379
--- /dev/null
+++ b/lld/test/COFF/exported-dllmain.test
@@ -0,0 +1,57 @@
+REQUIRES: x86
+RUN: split-file %s %t.dir && cd %t.dir
+
+RUN: llvm-mc -filetype=obj -triple=x86_64-windows a.s -o a.obj
+
+RUN: llvm-mc -filetype=obj -triple=x86_64-windows b1.s -o b1.obj
+RUN: llvm-mc -filetype=obj -triple=x86_64-windows b2.s -o b2.obj
+
+### This is the line where our problem occurs. Here, we export the DllMain symbol which shouldn't happen normally.
+RUN: lld-link b1.obj b2.obj -out:b.dll -dll -implib:b.lib -entry:DllMain -export:bar -export:DllMain
+
+RUN: llvm-mc -filetype=obj -triple=x86_64-windows c.s -o c.obj
+RUN: lld-link -lib c.obj -out:c.lib
+
+### Later, if b.lib is provided before other libs/objs that export DllMain statically, we previously were using the dllimported DllMain from b.lib, which is wrong.
+RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain 2>&1 | FileCheck -check-prefix=WARN %s
+RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain -ignore:exporteddllmain 2>&1 | FileCheck -check-prefix=IGNORED --allow-empty %s
+RUN: llvm-objdump --private-headers -d out.dll | FileCheck -check-prefix=DISASM %s
+
+WARN: lld-link: warning: b.lib: skipping exported DllMain symbol [exporteddllmain]
+IGNORED-NOT: lld-link: warning: b.lib: skipping exported DllMain symbol [exporteddllmain]
+
+DISASM: The Import Tables:
+DISASM: DLL Name: b.dll
+DISASM-NOT: DllMain
+DISASM: bar
+DISASM: Disassembly of section .text:
+DISASM-EMPTY:
+DISASM: b8 01 00 00 00 movl $0x1, %eax
+DISASM-NEXT: c3 retq
+
+#--- a.s
+ .text
+ .globl foo
+foo:
+ call *__imp_bar(%rip)
+ ret
+
+#--- b1.s
+ .text
+ .globl bar
+bar:
+ ret
+
+#--- b2.s
+ .text
+ .globl DllMain
+DllMain:
+ xor %eax, %eax
+ ret
+
+#--- c.s
+ .text
+ .globl DllMain
+DllMain:
+ movl $1, %eax
+ ret
|
|
/cherry-pick e63de82 |
|
Failed to cherry-pick: e63de82 https://github.com/llvm/llvm-project/actions/runs/16025725389 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
…6610) This is a workaround for llvm#82050 by skipping the `DllMain` symbol if seen in aimport library. If this situation occurs, after this commit a warning will also be displayed. The warning can be silenced with `/ignore:exporteddllmain`
| // '/ignore:exporteddllmain' | ||
| Warn(ctx) | ||
| << file->getFileName() | ||
| << ": skipping exported DllMain symbol [exporteddllmain]\nNOTE: this " |
There was a problem hiding this comment.
Sorry I'm a little late to the party here, but I think the option naming (and likewise the internal variables, but they're less effort to change later) feels a bit backwards here.
From the point of view of this link, there's an imported DllMain that we're ignoring. (This import lib entry probably exists because some DLL erroneously does export it, yes, but from the point of view of this linking, the issue is about an imported DllMain, not an issue about ourselves exporting DllMain.)
| for (const Archive::Symbol &sym : file->symbols()) { | ||
| // If the DllMain symbol was exported by mistake, skip importing it | ||
| // otherwise we might end up with a import thunk in the final binary which | ||
| // is wrong. |
There was a problem hiding this comment.
I find this comment a bit hand-wavy. Obviously, calling another DLL's DllMain rathern than our own is not going to be doing the right thing - it's not so much about an import thunk or so, right? So I'd rather just say this:
// If an import library provides the DllMain symbol, skip importing it, as we should be using our own DllMain, not another DLL's DllMain.
| // If the DllMain symbol was exported by mistake, skip importing it | ||
| // otherwise we might end up with a import thunk in the final binary which | ||
| // is wrong. | ||
| if (sym.getName() == "__imp_DllMain" || sym.getName() == "DllMain") { |
There was a problem hiding this comment.
This seems to be missing the relevant prefix handling for i386?
|
I opened #147152 to address your comments @mstorsjo. |
Nah, no need to revert - your incremental fix looks mostly good, just one comment on it. |
This is a workaround for #82050 by skipping importing the
DllMainsymbol from import libraries. If this situation occurs, after this PR a warning will also be displayed. The warning can be silenced with/ignore:exporteddllmain