[LLD][COFF] Add support for MinGW auto-export on ARM64X #125862

cjacek · 2025-02-05T14:21:15Z

Export all symbols from both EC and native symbol tables. If an explicit export is present in either symbol table, auto-export is disabled for both.

llvmbot · 2025-02-05T14:21:51Z

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

@llvm/pr-subscribers-lld-coff

Author: Jacek Caban (cjacek)

Changes

Export all symbols from both EC and native symbol tables. If an explicit export is present in only one symbol table, auto-export for the other symbol table remains unaffected.

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

4 Files Affected:

(modified) lld/COFF/Driver.cpp (+13-12)
(modified) lld/COFF/Driver.h (+2-1)
(modified) lld/COFF/MinGW.cpp (+4-5)
(modified) lld/COFF/MinGW.h (+2-2)

diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 979c0ae4962732..599ee54c2b0703 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -1377,18 +1377,19 @@ void LinkerDriver::pullArm64ECIcallHelper() {
 // explicitly specified. The automatic behavior can be disabled using the
 // -exclude-all-symbols option, so that lld-link behaves like link.exe rather
 // than MinGW in the case that nothing is explicitly exported.
-void LinkerDriver::maybeExportMinGWSymbols(const opt::InputArgList &args) {
+void LinkerDriver::maybeExportMinGWSymbols(SymbolTable &symtab,
+                                           const opt::InputArgList &args) {
   if (!args.hasArg(OPT_export_all_symbols)) {
     if (!ctx.config.dll)
       return;
 
-    if (!ctx.symtab.exports.empty())
+    if (!symtab.exports.empty())
       return;
     if (args.hasArg(OPT_exclude_all_symbols))
       return;
   }
 
-  AutoExporter exporter(ctx, excludedSymbols);
+  AutoExporter exporter(symtab, excludedSymbols);
 
   for (auto *arg : args.filtered(OPT_wholearchive_file))
     if (std::optional<StringRef> path = findFile(arg->getValue()))
@@ -1398,10 +1399,10 @@ void LinkerDriver::maybeExportMinGWSymbols(const opt::InputArgList &args) {
     SmallVector<StringRef, 2> vec;
     StringRef(arg->getValue()).split(vec, ',');
     for (StringRef sym : vec)
-      exporter.addExcludedSymbol(ctx.symtab.mangle(sym));
+      exporter.addExcludedSymbol(symtab.mangle(sym));
   }
 
-  ctx.symtab.forEachSymbol([&](Symbol *s) {
+  symtab.forEachSymbol([&](Symbol *s) {
     auto *def = dyn_cast<Defined>(s);
     if (!exporter.shouldExport(def))
       return;
@@ -1418,7 +1419,7 @@ void LinkerDriver::maybeExportMinGWSymbols(const opt::InputArgList &args) {
       if (!(c->getOutputCharacteristics() & IMAGE_SCN_MEM_EXECUTE))
         e.data = true;
     s->isUsedInRegularObj = true;
-    ctx.symtab.exports.push_back(e);
+    symtab.exports.push_back(e);
   });
 }
 
@@ -2613,14 +2614,14 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   if (errorCount())
     return;
 
-  ctx.forEachSymtab([](SymbolTable &symtab) {
+  ctx.forEachSymtab([&](SymbolTable &symtab) {
     symtab.hadExplicitExports = !symtab.exports.empty();
+    if (config->mingw) {
+      // In MinGW, all symbols are automatically exported if no symbols
+      // are chosen to be exported.
+      maybeExportMinGWSymbols(symtab, args);
+    }
   });
-  if (config->mingw) {
-    // In MinGW, all symbols are automatically exported if no symbols
-    // are chosen to be exported.
-    maybeExportMinGWSymbols(args);
-  }
 
   // Do LTO by compiling bitcode input files to a set of native COFF files then
   // link those files (unless -thinlto-index-only was given, in which case we
diff --git a/lld/COFF/Driver.h b/lld/COFF/Driver.h
index 58dc5458e9a544..58ec1d7aabe5c4 100644
--- a/lld/COFF/Driver.h
+++ b/lld/COFF/Driver.h
@@ -161,7 +161,8 @@ class LinkerDriver {
   // trees into one resource tree.
   void convertResources();
 
-  void maybeExportMinGWSymbols(const llvm::opt::InputArgList &args);
+  void maybeExportMinGWSymbols(SymbolTable &symtab,
+                               const llvm::opt::InputArgList &args);
 
   // We don't want to add the same file more than once.
   // Files are uniquified by their filesystem and file number.
diff --git a/lld/COFF/MinGW.cpp b/lld/COFF/MinGW.cpp
index 76f5a0a7500b9d..a6407bc279200e 100644
--- a/lld/COFF/MinGW.cpp
+++ b/lld/COFF/MinGW.cpp
@@ -25,9 +25,8 @@ using namespace lld;
 using namespace lld::coff;
 
 AutoExporter::AutoExporter(
-    COFFLinkerContext &ctx,
-    const llvm::DenseSet<StringRef> &manualExcludeSymbols)
-    : manualExcludeSymbols(manualExcludeSymbols), ctx(ctx) {
+    SymbolTable &symtab, const llvm::DenseSet<StringRef> &manualExcludeSymbols)
+    : manualExcludeSymbols(manualExcludeSymbols), symtab(symtab) {
   excludeLibs = {
       "libgcc",
       "libgcc_s",
@@ -84,7 +83,7 @@ AutoExporter::AutoExporter(
       "_NULL_THUNK_DATA",
   };
 
-  if (ctx.config.machine == I386) {
+  if (symtab.machine == I386) {
     excludeSymbols = {
         "__NULL_IMPORT_DESCRIPTOR",
         "__pei386_runtime_relocator",
@@ -151,7 +150,7 @@ bool AutoExporter::shouldExport(Defined *sym) const {
       return false;
 
   // If a corresponding __imp_ symbol exists and is defined, don't export it.
-  if (ctx.symtab.find(("__imp_" + sym->getName()).str()))
+  if (symtab.find(("__imp_" + sym->getName()).str()))
     return false;
 
   // Check that file is non-null before dereferencing it, symbols not
diff --git a/lld/COFF/MinGW.h b/lld/COFF/MinGW.h
index ffa500b234777b..265beb315c9a71 100644
--- a/lld/COFF/MinGW.h
+++ b/lld/COFF/MinGW.h
@@ -25,7 +25,7 @@ class COFFLinkerContext;
 // symbols for MinGW.
 class AutoExporter {
 public:
-  AutoExporter(COFFLinkerContext &ctx,
+  AutoExporter(SymbolTable &symtab,
                const llvm::DenseSet<StringRef> &manualExcludeSymbols);
 
   void addWholeArchive(StringRef path);
@@ -42,7 +42,7 @@ class AutoExporter {
   bool shouldExport(Defined *sym) const;
 
 private:
-  COFFLinkerContext &ctx;
+  SymbolTable &symtab;
 };
 
 void writeDefFile(COFFLinkerContext &, StringRef name,

cjacek · 2025-02-05T14:22:02Z

The decision to handle explicit exports separately for each symbol table is somewhat arbitrary, but it provides more flexibility. If users prefer a different behavior, they can explicitly control auto-exports using command-line arguments.

(Handling ARM64X in MinGW is still undecided, but I’m sending these changes now to avoid inconsistencies caused by silently ignoring them.)

mstorsjo · 2025-02-05T22:24:17Z

The decision to handle explicit exports separately for each symbol table is somewhat arbitrary, but it provides more flexibility. If users prefer a different behavior, they can explicitly control auto-exports using command-line arguments.

(Handling ARM64X in MinGW is still undecided, but I’m sending these changes now to avoid inconsistencies caused by silently ignoring them.)

Indeed, this case is quite unexplored yet...

Intuitively, it feels strange to autoexport symbols in one namespace but not in the other one. But code wise, it looks intuitive and straightforward in this form...

I guess the practical case is if we'd have e.g. a dllexport directive in one object file in one namespace, but none in the other one. Autoexporting all symbols on one side but only one single symbol in the other one feels a bit unexpected then, even if it may be logical. I wonder if it would be more intuitive if we'd end up with 1 symbol on one side and 0 on the other, in that case - what do you think?

In any case, I don't see any testcase added here?

Export all symbols from both EC and native symbol tables. If an explicit export is present in either symbol table, auto-export is disabled for both.

cjacek · 2025-02-06T12:41:00Z

Indeed, this case is quite unexplored yet...

Intuitively, it feels strange to autoexport symbols in one namespace but not in the other one. But code wise, it looks intuitive and straightforward in this form...

I guess the practical case is if we'd have e.g. a dllexport directive in one object file in one namespace, but none in the other one. Autoexporting all symbols on one side but only one single symbol in the other one feels a bit unexpected then, even if it may be logical. I wonder if it would be more intuitive if we'd end up with 1 symbol on one side and 0 on the other, in that case - what do you think?

I can see arguments for both approaches. Initially, I leaned toward treating them separately since users could override that with -export-all-symbols, whereas there was no way to specify auto-export for only one view. However, this is a weak argument, as the use case is likely unrealistic. I’ve updated the logic in the new version.

In any case, I don't see any testcase added here?

You're right, I forgot to commit it. Sorry about that!

mstorsjo

LGTM, thanks!

mstorsjo · 2025-02-06T12:53:02Z

lld/test/COFF/arm64x-export-all.s

+// EXP-NEXT: AddressSize: 64bit
+// EXP-NEXT: Export {
+// EXP-NEXT:   Ordinal: 1
+// EXP-NEXT:   Name: _load_config_used


Side note; I guess this is a symbol we could include in excludeSymbols, as it doesn't make sense to autoexport this one.

Yes, I will do that in a followup. Thanks!

mstorsjo · 2025-02-06T12:53:40Z

lld/test/COFF/arm64x-export-all.s

+// EXP-NEXT:   }
+// EXP-NEXT:   Export {
+// EXP-NEXT:     Ordinal: 3
+// EXP-NEXT:     Name: __os_arm64x_dispatch_ret


Same for all these arm64ec internals - we probably don't want to export them. But that's a difference concern from this PR anyway.

Export all symbols from both EC and native symbol tables. If an explicit export is present in either symbol table, auto-export is disabled for both.

cjacek requested a review from mstorsjo February 5, 2025 14:21

llvmbot added lld lld:COFF platform:windows labels Feb 5, 2025

[LLD][COFF] Add MinGW auto-export support for ARM64X

eecab63

Export all symbols from both EC and native symbol tables. If an explicit export is present in either symbol table, auto-export is disabled for both.

cjacek force-pushed the arm64x-autoexport branch from a549289 to eecab63 Compare February 6, 2025 12:34

mstorsjo approved these changes Feb 6, 2025

View reviewed changes

cjacek merged commit f729477 into llvm:main Feb 6, 2025
8 checks passed

cjacek deleted the arm64x-autoexport branch February 6, 2025 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[LLD][COFF] Add support for MinGW auto-export on ARM64X #125862

[LLD][COFF] Add support for MinGW auto-export on ARM64X #125862

Uh oh!

cjacek commented Feb 5, 2025 •

edited

Loading

Uh oh!

llvmbot commented Feb 5, 2025 •

edited

Loading

Uh oh!

cjacek commented Feb 5, 2025

Uh oh!

mstorsjo commented Feb 5, 2025

Uh oh!

cjacek commented Feb 6, 2025

Uh oh!

mstorsjo left a comment

Uh oh!

mstorsjo Feb 6, 2025

Uh oh!

cjacek Feb 6, 2025

Uh oh!

mstorsjo Feb 6, 2025

Uh oh!

Uh oh!

Uh oh!

[LLD][COFF] Add support for MinGW auto-export on ARM64X #125862

[LLD][COFF] Add support for MinGW auto-export on ARM64X #125862

Uh oh!

Conversation

cjacek commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjacek commented Feb 5, 2025

Uh oh!

mstorsjo commented Feb 5, 2025

Uh oh!

cjacek commented Feb 6, 2025

Uh oh!

mstorsjo left a comment

Choose a reason for hiding this comment

Uh oh!

mstorsjo Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

cjacek Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

mstorsjo Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cjacek commented Feb 5, 2025 •

edited

Loading

llvmbot commented Feb 5, 2025 •

edited

Loading