[LLD][COFF] Use EC symbol table for exports defined in module definition files

[cjacek]

No description provided.

[llvmbot]

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

@llvm/pr-subscribers-lld

Author: Jacek Caban (cjacek)

Changes

---

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

5 Files Affected:

• (modified) lld/COFF/Driver.cpp (+1-63)
• (modified) lld/COFF/Driver.h (-2)
• (modified) lld/COFF/SymbolTable.cpp (+63)
• (modified) lld/COFF/SymbolTable.h (+1)
• (modified) lld/test/COFF/arm64x-export.test (+12)

diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 4e0678282eed01..3fde9c84d977db 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -30,7 +30,6 @@
 #include "llvm/LTO/LTO.h"
 #include "llvm/Object/ArchiveWriter.h"
 #include "llvm/Object/COFFImportFile.h"
-#include "llvm/Object/COFFModuleDefinition.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
@@ -1012,67 +1011,6 @@ void LinkerDriver::createImportLibrary(bool asLib) {
   }
 }
 
-void LinkerDriver::parseModuleDefs(StringRef path) {
-  llvm::TimeTraceScope timeScope("Parse def file");
-  std::unique_ptr<MemoryBuffer> mb =
-      CHECK(MemoryBuffer::getFile(path, /*IsText=*/false,
-                                  /*RequiresNullTerminator=*/false,
-                                  /*IsVolatile=*/true),
-            "could not open " + path);
-  COFFModuleDefinition m = check(parseCOFFModuleDefinition(
-      mb->getMemBufferRef(), ctx.config.machine, ctx.config.mingw));
-
-  // Include in /reproduce: output if applicable.
-  ctx.driver.takeBuffer(std::move(mb));
-
-  if (ctx.config.outputFile.empty())
-    ctx.config.outputFile = std::string(saver().save(m.OutputFile));
-  ctx.config.importName = std::string(saver().save(m.ImportName));
-  if (m.ImageBase)
-    ctx.config.imageBase = m.ImageBase;
-  if (m.StackReserve)
-    ctx.config.stackReserve = m.StackReserve;
-  if (m.StackCommit)
-    ctx.config.stackCommit = m.StackCommit;
-  if (m.HeapReserve)
-    ctx.config.heapReserve = m.HeapReserve;
-  if (m.HeapCommit)
-    ctx.config.heapCommit = m.HeapCommit;
-  if (m.MajorImageVersion)
-    ctx.config.majorImageVersion = m.MajorImageVersion;
-  if (m.MinorImageVersion)
-    ctx.config.minorImageVersion = m.MinorImageVersion;
-  if (m.MajorOSVersion)
-    ctx.config.majorOSVersion = m.MajorOSVersion;
-  if (m.MinorOSVersion)
-    ctx.config.minorOSVersion = m.MinorOSVersion;
-
-  for (COFFShortExport e1 : m.Exports) {
-    Export e2;
-    // Renamed exports are parsed and set as "ExtName = Name". If Name has
-    // the form "OtherDll.Func", it shouldn't be a normal exported
-    // function but a forward to another DLL instead. This is supported
-    // by both MS and GNU linkers.
-    if (!e1.ExtName.empty() && e1.ExtName != e1.Name &&
-        StringRef(e1.Name).contains('.')) {
-      e2.name = saver().save(e1.ExtName);
-      e2.forwardTo = saver().save(e1.Name);
-    } else {
-      e2.name = saver().save(e1.Name);
-      e2.extName = saver().save(e1.ExtName);
-    }
-    e2.exportAs = saver().save(e1.ExportAs);
-    e2.importName = saver().save(e1.ImportName);
-    e2.ordinal = e1.Ordinal;
-    e2.noname = e1.Noname;
-    e2.data = e1.Data;
-    e2.isPrivate = e1.Private;
-    e2.constant = e1.Constant;
-    e2.source = ExportSource::ModuleDefinition;
-    ctx.symtab.exports.push_back(e2);
-  }
-}
-
 void LinkerDriver::enqueueTask(std::function<void()> task) {
   taskQueue.push_back(std::move(task));
 }
@@ -2352,7 +2290,7 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   // Handle /def
   if (auto *arg = args.getLastArg(OPT_deffile)) {
     // parseModuleDefs mutates Config object.
-    parseModuleDefs(arg->getValue());
+    mainSymtab.parseModuleDefs(arg->getValue());
   }
 
   // Handle generation of import library from a def file.
diff --git a/lld/COFF/Driver.h b/lld/COFF/Driver.h
index 12724cbd1eef49..58dc5458e9a544 100644
--- a/lld/COFF/Driver.h
+++ b/lld/COFF/Driver.h
@@ -143,8 +143,6 @@ class LinkerDriver {
   // Used by the resolver to parse .drectve section contents.
   void parseDirectives(InputFile *file);
 
-  void parseModuleDefs(StringRef path);
-
   // Parse an /order file. If an option is given, the linker places COMDAT
   // sections int he same order as their names appear in the given file.
   void parseOrderFile(StringRef arg);
diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp
index ecccc7d6ed70c7..32ea4a5b2e1fc3 100644
--- a/lld/COFF/SymbolTable.cpp
+++ b/lld/COFF/SymbolTable.cpp
@@ -20,6 +20,7 @@
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/LTO/LTO.h"
+#include "llvm/Object/COFFModuleDefinition.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/GlobPattern.h"
 #include "llvm/Support/Parallel.h"
@@ -29,6 +30,7 @@
 
 using namespace llvm;
 using namespace llvm::COFF;
+using namespace llvm::object;
 using namespace llvm::support;
 
 namespace lld::coff {
@@ -1253,6 +1255,67 @@ void SymbolTable::assignExportOrdinals() {
                << Twine(std::numeric_limits<uint16_t>::max()) << ")";
 }
 
+void SymbolTable::parseModuleDefs(StringRef path) {
+  llvm::TimeTraceScope timeScope("Parse def file");
+  std::unique_ptr<MemoryBuffer> mb =
+      CHECK(MemoryBuffer::getFile(path, /*IsText=*/false,
+                                  /*RequiresNullTerminator=*/false,
+                                  /*IsVolatile=*/true),
+            "could not open " + path);
+  COFFModuleDefinition m = check(parseCOFFModuleDefinition(
+      mb->getMemBufferRef(), machine, ctx.config.mingw));
+
+  // Include in /reproduce: output if applicable.
+  ctx.driver.takeBuffer(std::move(mb));
+
+  if (ctx.config.outputFile.empty())
+    ctx.config.outputFile = std::string(saver().save(m.OutputFile));
+  ctx.config.importName = std::string(saver().save(m.ImportName));
+  if (m.ImageBase)
+    ctx.config.imageBase = m.ImageBase;
+  if (m.StackReserve)
+    ctx.config.stackReserve = m.StackReserve;
+  if (m.StackCommit)
+    ctx.config.stackCommit = m.StackCommit;
+  if (m.HeapReserve)
+    ctx.config.heapReserve = m.HeapReserve;
+  if (m.HeapCommit)
+    ctx.config.heapCommit = m.HeapCommit;
+  if (m.MajorImageVersion)
+    ctx.config.majorImageVersion = m.MajorImageVersion;
+  if (m.MinorImageVersion)
+    ctx.config.minorImageVersion = m.MinorImageVersion;
+  if (m.MajorOSVersion)
+    ctx.config.majorOSVersion = m.MajorOSVersion;
+  if (m.MinorOSVersion)
+    ctx.config.minorOSVersion = m.MinorOSVersion;
+
+  for (COFFShortExport e1 : m.Exports) {
+    Export e2;
+    // Renamed exports are parsed and set as "ExtName = Name". If Name has
+    // the form "OtherDll.Func", it shouldn't be a normal exported
+    // function but a forward to another DLL instead. This is supported
+    // by both MS and GNU linkers.
+    if (!e1.ExtName.empty() && e1.ExtName != e1.Name &&
+        StringRef(e1.Name).contains('.')) {
+      e2.name = saver().save(e1.ExtName);
+      e2.forwardTo = saver().save(e1.Name);
+    } else {
+      e2.name = saver().save(e1.Name);
+      e2.extName = saver().save(e1.ExtName);
+    }
+    e2.exportAs = saver().save(e1.ExportAs);
+    e2.importName = saver().save(e1.ImportName);
+    e2.ordinal = e1.Ordinal;
+    e2.noname = e1.Noname;
+    e2.data = e1.Data;
+    e2.isPrivate = e1.Private;
+    e2.constant = e1.Constant;
+    e2.source = ExportSource::ModuleDefinition;
+    exports.push_back(e2);
+  }
+}
+
 Symbol *SymbolTable::addUndefined(StringRef name) {
   return addUndefined(name, nullptr, false);
 }
diff --git a/lld/COFF/SymbolTable.h b/lld/COFF/SymbolTable.h
index e5b02ce5904c49..c8d7251838842f 100644
--- a/lld/COFF/SymbolTable.h
+++ b/lld/COFF/SymbolTable.h
@@ -160,6 +160,7 @@ class SymbolTable {
 
   void fixupExports();
   void assignExportOrdinals();
+  void parseModuleDefs(StringRef path);
 
   // Iterates symbols in non-determinstic hash table order.
   template <typename T> void forEachSymbol(T callback) {
diff --git a/lld/test/COFF/arm64x-export.test b/lld/test/COFF/arm64x-export.test
index 526be633973581..a78b291cedbe10 100644
--- a/lld/test/COFF/arm64x-export.test
+++ b/lld/test/COFF/arm64x-export.test
@@ -55,6 +55,13 @@ RUN:          loadconfig-arm64.obj loadconfig-arm64ec.obj arm64ec-drectve.obj -n
 RUN: llvm-objdump -d out-drectve-ec.dll | FileCheck --check-prefix=DISASM-EC %s
 RUN: llvm-readobj --headers --coff-exports out-drectve-ec.dll | FileCheck --check-prefix=EXPORTS-EC %s
 
+# A command-line def file applies only to EC exports.
+
+RUN: lld-link -machine:arm64x -dll -out:out-def-ec.dll arm64ec-func.obj arm64-func.obj \
+RUN:          loadconfig-arm64.obj loadconfig-arm64ec.obj -def:func.def -noentry
+RUN: llvm-objdump -d out-def-ec.dll | FileCheck --check-prefix=DISASM-EC %s
+RUN: llvm-readobj --headers --coff-exports out-def-ec.dll | FileCheck --check-prefix=EXPORTS-EC %s
+
 # Export using the EC .edata section.
 
 RUN: lld-link -machine:arm64x -dll -out:out-edata-ec.dll arm64ec-func.obj arm64-func.obj \
@@ -227,3 +234,8 @@ funcname_func:
 
 name:
     .asciz "out-edata.dll"
+
+#--- func.def
+LIBRARY out.dll
+EXPORTS
+        func

[mstorsjo]

LGTM

On the earlier note on "lots of code that used to live elsewhere now is moved into SymbolTable", I wonder if we should make a separate class out of that (e.g. as a superclass to SymbolTable) which could live in a separate file, like LinkingNamespace or something like that ("linking namespace" is, strictly speaking, somewhat synonymous to symbol table though, so perhaps it's not the best name). So that things that belong more to "global linking state", but which is split per EC/native namespace could go there.

(This is not urgent, we can look into this after getting the relevant functionality in place.)

[cjacek]

I experimented with something similar in an earlier version of ARM64X support, where I called it COFFNamespace, see cjacek@de3b848#diff-e0126077b0ef020c4220f67717f0c3a7040f30473484ef2a2e702a5daddb42ab. At that time, I didn't move functions to that class and instead stored the symbol table inside it rather than using inheritance, which made it feel like an unnecessary complication. Your suggested approach seems good, I’ll revisit it.