Skip to content

[llvm-dlltool] Handle import renaming using other name types, when possible #98228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Jul 9, 2024

This avoids needing to use weak aliases for these cases. (Weak
aliases only work if there's another, regular import entry that
provide the desired symbol from the DLL.)

@mstorsjo
Copy link
Member Author

mstorsjo commented Jul 9, 2024

This goes on top of #98226, only the topmost commit is for review here.

@llvmbot
Copy link
Member

llvmbot commented Jul 9, 2024

@llvm/pr-subscribers-llvm-binary-utilities

Author: Martin Storsjö (mstorsjo)

Changes

This avoids needing to use weak aliases for these cases. (Weak
aliases only work if there's another, regular import entry that
provide the desired symbol from the DLL.)


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

6 Files Affected:

  • (modified) llvm/include/llvm/Object/COFFImportFile.h (+5-4)
  • (modified) llvm/lib/Object/COFFImportFile.cpp (+43-15)
  • (modified) llvm/lib/Object/COFFModuleDefinition.cpp (-2)
  • (modified) llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp (+3-2)
  • (modified) llvm/test/tools/llvm-dlltool/coff-decorated.def (+16)
  • (modified) llvm/test/tools/llvm-dlltool/coff-weak-exports.def (+4)
diff --git a/llvm/include/llvm/Object/COFFImportFile.h b/llvm/include/llvm/Object/COFFImportFile.h
index 649fb4930934d..e91d5a9b3198a 100644
--- a/llvm/include/llvm/Object/COFFImportFile.h
+++ b/llvm/include/llvm/Object/COFFImportFile.h
@@ -135,10 +135,11 @@ struct COFFShortExport {
 /// linking both ARM64EC and pure ARM64 objects, and the linker will pick only
 /// the exports relevant to the target platform. For non-hybrid targets,
 /// the NativeExports parameter should not be used.
-Error writeImportLibrary(
-    StringRef ImportName, StringRef Path, ArrayRef<COFFShortExport> Exports,
-    COFF::MachineTypes Machine, bool MinGW,
-    ArrayRef<COFFShortExport> NativeExports = std::nullopt);
+Error writeImportLibrary(StringRef ImportName, StringRef Path,
+                         ArrayRef<COFFShortExport> Exports,
+                         COFF::MachineTypes Machine, bool MinGW,
+                         ArrayRef<COFFShortExport> NativeExports = std::nullopt,
+                         bool AddUnderscores = true);
 
 } // namespace object
 } // namespace llvm
diff --git a/llvm/lib/Object/COFFImportFile.cpp b/llvm/lib/Object/COFFImportFile.cpp
index fd8aca393e90b..2736179b65c7c 100644
--- a/llvm/lib/Object/COFFImportFile.cpp
+++ b/llvm/lib/Object/COFFImportFile.cpp
@@ -52,18 +52,12 @@ StringRef COFFImportFile::getFileFormatName() const {
   }
 }
 
-StringRef COFFImportFile::getExportName() const {
-  const coff_import_header *hdr = getCOFFImportHeader();
-  StringRef name = Data.getBuffer().substr(sizeof(*hdr)).split('\0').first;
-
+static StringRef applyNameType(ImportNameType Type, StringRef name) {
   auto ltrim1 = [](StringRef s, StringRef chars) {
     return !s.empty() && chars.contains(s[0]) ? s.substr(1) : s;
   };
 
-  switch (hdr->getNameType()) {
-  case IMPORT_ORDINAL:
-    name = "";
-    break;
+  switch (Type) {
   case IMPORT_NAME_NOPREFIX:
     name = ltrim1(name, "?@_");
     break;
@@ -71,6 +65,24 @@ StringRef COFFImportFile::getExportName() const {
     name = ltrim1(name, "?@_");
     name = name.substr(0, name.find('@'));
     break;
+  default:
+    break;
+  }
+  return name;
+}
+
+StringRef COFFImportFile::getExportName() const {
+  const coff_import_header *hdr = getCOFFImportHeader();
+  StringRef name = Data.getBuffer().substr(sizeof(*hdr)).split('\0').first;
+
+  switch (hdr->getNameType()) {
+  case IMPORT_ORDINAL:
+    name = "";
+    break;
+  case IMPORT_NAME_NOPREFIX:
+  case IMPORT_NAME_UNDECORATE:
+    name = applyNameType(static_cast<ImportNameType>(hdr->getNameType()), name);
+    break;
   case IMPORT_NAME_EXPORTAS: {
     // Skip DLL name
     name = Data.getBuffer().substr(sizeof(*hdr) + name.size() + 1);
@@ -645,7 +657,8 @@ NewArchiveMember ObjectFactory::createWeakExternal(StringRef Sym,
 Error writeImportLibrary(StringRef ImportName, StringRef Path,
                          ArrayRef<COFFShortExport> Exports,
                          MachineTypes Machine, bool MinGW,
-                         ArrayRef<COFFShortExport> NativeExports) {
+                         ArrayRef<COFFShortExport> NativeExports,
+                         bool AddUnderscores) {
 
   MachineTypes NativeMachine = Machine;
   if (isArm64EC(Machine)) {
@@ -690,12 +703,6 @@ Error writeImportLibrary(StringRef ImportName, StringRef Path,
         Name.swap(*ReplacedName);
       }
 
-      if (!E.ImportName.empty() && Name != E.ImportName) {
-        Members.push_back(OF.createWeakExternal(E.ImportName, Name, false, M));
-        Members.push_back(OF.createWeakExternal(E.ImportName, Name, true, M));
-        continue;
-      }
-
       ImportNameType NameType;
       std::string ExportName;
       if (E.Noname) {
@@ -703,6 +710,27 @@ Error writeImportLibrary(StringRef ImportName, StringRef Path,
       } else if (!E.ExportAs.empty()) {
         NameType = IMPORT_NAME_EXPORTAS;
         ExportName = E.ExportAs;
+      } else if (!E.ImportName.empty()) {
+        if (Machine == IMAGE_FILE_MACHINE_I386 &&
+            applyNameType(IMPORT_NAME_UNDECORATE, Name) == E.ImportName)
+          NameType = IMPORT_NAME_UNDECORATE;
+        else if (Machine == IMAGE_FILE_MACHINE_I386 &&
+                 applyNameType(IMPORT_NAME_NOPREFIX, Name) == E.ImportName)
+          NameType = IMPORT_NAME_NOPREFIX;
+        else if (Name == E.ImportName)
+          NameType = IMPORT_NAME;
+        else {
+          StringRef Prefix = "";
+          if (Machine == IMAGE_FILE_MACHINE_I386 && AddUnderscores)
+            Prefix = "_";
+
+          if (ImportType == IMPORT_CODE)
+            Members.push_back(OF.createWeakExternal(
+                (Prefix + E.ImportName).str(), Name, false, M));
+          Members.push_back(OF.createWeakExternal((Prefix + E.ImportName).str(),
+                                                  Name, true, M));
+          continue;
+        }
       } else {
         NameType = getNameType(SymbolName, E.Name, M, MinGW);
       }
diff --git a/llvm/lib/Object/COFFModuleDefinition.cpp b/llvm/lib/Object/COFFModuleDefinition.cpp
index 0c0bef1319e44..82c18539658e8 100644
--- a/llvm/lib/Object/COFFModuleDefinition.cpp
+++ b/llvm/lib/Object/COFFModuleDefinition.cpp
@@ -282,8 +282,6 @@ class Parser {
       if (Tok.K == EqualEqual) {
         read();
         E.ImportName = std::string(Tok.Value);
-        if (AddUnderscores && !isDecorated(E.ImportName, MingwDef))
-          E.ImportName = std::string("_").append(E.ImportName);
         continue;
       }
       // EXPORTAS must be at the end of export definition
diff --git a/llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp b/llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
index 15e4cac08cd4e..012ad246888f9 100644
--- a/llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
+++ b/llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
@@ -243,8 +243,9 @@ int llvm::dlltoolDriverMain(llvm::ArrayRef<const char *> ArgsArr) {
   }
 
   std::string Path = std::string(Args.getLastArgValue(OPT_l));
-  if (!Path.empty() && writeImportLibrary(OutputFile, Path, Exports, Machine,
-                                          /*MinGW=*/true, NativeExports))
+  if (!Path.empty() &&
+      writeImportLibrary(OutputFile, Path, Exports, Machine,
+                         /*MinGW=*/true, NativeExports, AddUnderscores))
     return 1;
   return 0;
 }
diff --git a/llvm/test/tools/llvm-dlltool/coff-decorated.def b/llvm/test/tools/llvm-dlltool/coff-decorated.def
index fc81f23d09d6c..773e3762cc3d7 100644
--- a/llvm/test/tools/llvm-dlltool/coff-decorated.def
+++ b/llvm/test/tools/llvm-dlltool/coff-decorated.def
@@ -13,6 +13,10 @@ StdcallExportName@4=StdcallInternalFunction@4
 OtherStdcallExportName@4=CdeclInternalFunction
 CdeclExportName=StdcallInternalFunction@4
 
+NoprefixStdcall@4 == NoprefixStdcall@4
+DecoratedStdcall@4 == _DecoratedStdcall@4
+UndecoratedStdcall@4 == UndecoratedStdcall
+
 ; CHECK:      Name type: noprefix
 ; CHECK-NEXT: Export name: CdeclFunction
 ; CHECK-NEXT: Symbol: __imp__CdeclFunction
@@ -43,3 +47,15 @@ CdeclExportName=StdcallInternalFunction@4
 ; CHECK-NEXT: Export name: CdeclExportName
 ; CHECK-NEXT: Symbol: __imp__CdeclExportName
 ; CHECK-NEXT: Symbol: _CdeclExportName
+; CHECK:      Name type: noprefix
+; CHECK-NEXT: Export name: NoprefixStdcall@4
+; CHECK-NEXT: Symbol: __imp__NoprefixStdcall@4
+; CHECK-NEXT: Symbol: _NoprefixStdcall@4
+; CHECK:      Name type: name
+; CHECK-NEXT: Export name: _DecoratedStdcall@4
+; CHECK-NEXT: Symbol: __imp__DecoratedStdcall@4
+; CHECK-NEXT: Symbol: _DecoratedStdcall@4
+; CHECK:      Name type: undecorate
+; CHECK-NEXT: Export name: UndecoratedStdcall
+; CHECK-NEXT: Symbol: __imp__UndecoratedStdcall@4
+; CHECK-NEXT: Symbol: _UndecoratedStdcall@4
diff --git a/llvm/test/tools/llvm-dlltool/coff-weak-exports.def b/llvm/test/tools/llvm-dlltool/coff-weak-exports.def
index dacc5f73530fd..67f0013bf170f 100644
--- a/llvm/test/tools/llvm-dlltool/coff-weak-exports.def
+++ b/llvm/test/tools/llvm-dlltool/coff-weak-exports.def
@@ -6,6 +6,7 @@
 LIBRARY test.dll
 EXPORTS
 TestFunction==AltTestFunction
+TestData DATA == AltTestData
 ; When creating an import library, the DLL internal function name of
 ; the implementation of a function isn't visible at all.
 ImpLibName = Implementation
@@ -20,6 +21,9 @@ ImpLibName3 = kernel32.Sleep
 ; CHECK-NEXT: W TestFunction
 ; CHECK:      U __imp_AltTestFunction
 ; CHECK-NEXT: W __imp_TestFunction
+; CHECK-NOT:  W TestData
+; CHECK:      U __imp_AltTestData
+; CHECK-NEXT: W __imp_TestData
 ; CHECK:      T ImpLibName
 ; CHECK-NEXT: T __imp_ImpLibName
 ; CHECK:      U AltTestFunction2

Copy link
Contributor

@cjacek cjacek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks correct to me. It feels quite hackish, but also clever. Perhaps a short comment in the code why we need it would be nice.

@mstorsjo mstorsjo force-pushed the dlltool-aliases-3 branch from 7f1ecac to 0d6df46 Compare July 11, 2024 21:47
@mstorsjo
Copy link
Member Author

The code looks correct to me. It feels quite hackish, but also clever. Perhaps a short comment in the code why we need it would be nice.

Thanks; I amended this with a comment now explaining why we try this.

…ssible

This avoids needing to use weak aliases for these cases. (Weak
aliases only work if there's another, regular import entry that
provide the desired symbol from the DLL.)
@mstorsjo mstorsjo force-pushed the dlltool-aliases-3 branch from 0d6df46 to 0fad0b9 Compare July 16, 2024 20:11
@mstorsjo mstorsjo merged commit 4469650 into llvm:main Jul 16, 2024
3 of 5 checks passed
@mstorsjo mstorsjo deleted the dlltool-aliases-3 branch July 16, 2024 20:17
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…ssible (#98228)

This avoids needing to use weak aliases for these cases. (Weak
aliases only work if there's another, regular import entry that
provide the desired symbol from the DLL.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants