Skip to content

[llvm-lib][llvm-dlltool] Fix handling of invalid ARM64EC function names #116250

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
Nov 15, 2024

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Nov 14, 2024

This is a follow-up to #115567. Emit an error for invalid function names, similar to MSVC's lib.exe behavior.

Returning an error from writeImportLibrary exposed bugs in error handling by its callers, which have been addressed in this patch.

This is a follow-up to llvm#115567. Emit an error for invalid function names, similar to MSVC's `lib.exe` behavior.

Returning an error from `writeImportLibrary` exposed bugs in error handling by its callers, which have been addressed in this patch.
@llvmbot
Copy link
Member

llvmbot commented Nov 14, 2024

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

Author: Jacek Caban (cjacek)

Changes

This is a follow-up to #115567. Emit an error for invalid function names, similar to MSVC's lib.exe behavior.

Returning an error from writeImportLibrary exposed bugs in error handling by its callers, which have been addressed in this patch.


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

5 Files Affected:

  • (modified) llvm/lib/Object/COFFImportFile.cpp (+8-1)
  • (modified) llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp (+9-3)
  • (modified) llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp (+9-4)
  • (added) llvm/test/tools/llvm-dlltool/arm64ec-invalid-name.test (+6)
  • (added) llvm/test/tools/llvm-lib/arm64ec-invalid-name.test (+6)
diff --git a/llvm/lib/Object/COFFImportFile.cpp b/llvm/lib/Object/COFFImportFile.cpp
index cc0a5da7e0d16a..ff3dcf9e13ffaf 100644
--- a/llvm/lib/Object/COFFImportFile.cpp
+++ b/llvm/lib/Object/COFFImportFile.cpp
@@ -756,8 +756,15 @@ Error writeImportLibrary(StringRef ImportName, StringRef Path,
           }
           Name = std::move(*MangledName);
         } else if (!E.Noname && ExportName.empty()) {
+          std::optional<std::string> DemangledName =
+              getArm64ECDemangledFunctionName(Name);
+          if (!DemangledName)
+            return make_error<StringError>(
+                StringRef(Twine("Invalid ARM64EC function name '" + Name + "'")
+                              .str()),
+                object_error::parse_failed);
           NameType = IMPORT_NAME_EXPORTAS;
-          ExportName = std::move(*getArm64ECDemangledFunctionName(Name));
+          ExportName = std::move(*DemangledName);
         }
       }
 
diff --git a/llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp b/llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
index 15e4cac08cd4ed..b3dcc0f9866842 100644
--- a/llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
+++ b/llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
@@ -243,8 +243,14 @@ 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))
-    return 1;
+  if (!Path.empty()) {
+    if (Error E = writeImportLibrary(OutputFile, Path, Exports, Machine,
+                                     /*MinGW=*/true, NativeExports)) {
+      handleAllErrors(std::move(E), [&](const ErrorInfoBase &EI) {
+        llvm::errs() << EI.message() << "\n";
+      });
+      return 1;
+    }
+  }
   return 0;
 }
diff --git a/llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp b/llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
index 07389a5ffb2b8a..2e0841ba02b543 100644
--- a/llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
+++ b/llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
@@ -419,10 +419,15 @@ int llvm::libDriverMain(ArrayRef<const char *> ArgsArr) {
       OutputFile = std::move(NativeDef->OutputFile);
     }
 
-    return writeImportLibrary(OutputFile, OutputPath, Def->Exports, LibMachine,
-                              /*MinGW=*/false, NativeExports)
-               ? 1
-               : 0;
+    if (Error E =
+            writeImportLibrary(OutputFile, OutputPath, Def->Exports, LibMachine,
+                               /*MinGW=*/false, NativeExports)) {
+      handleAllErrors(std::move(E), [&](const ErrorInfoBase &EI) {
+        llvm::errs() << OutputPath << ": " << EI.message() << "\n";
+      });
+      return 1;
+    }
+    return 0;
   }
 
   // If no input files and not told otherwise, silently do nothing to match
diff --git a/llvm/test/tools/llvm-dlltool/arm64ec-invalid-name.test b/llvm/test/tools/llvm-dlltool/arm64ec-invalid-name.test
new file mode 100644
index 00000000000000..9a32a7afd5e200
--- /dev/null
+++ b/llvm/test/tools/llvm-dlltool/arm64ec-invalid-name.test
@@ -0,0 +1,6 @@
+; RUN: not llvm-dlltool -m arm64ec -d %s -l %t.lib 2>&1 | FileCheck %s
+; CHECK: Invalid ARM64EC function name '?func'
+
+LIBRARY test.dll
+EXPORTS
+        ?func
diff --git a/llvm/test/tools/llvm-lib/arm64ec-invalid-name.test b/llvm/test/tools/llvm-lib/arm64ec-invalid-name.test
new file mode 100644
index 00000000000000..696b4c96eed22e
--- /dev/null
+++ b/llvm/test/tools/llvm-lib/arm64ec-invalid-name.test
@@ -0,0 +1,6 @@
+; RUN: not llvm-lib -machine:arm64ec -def:%s -out:%t.lib 2>&1 | FileCheck %s
+; CHECK: Invalid ARM64EC function name '?func'
+
+LIBRARY test.dll
+EXPORTS
+        ?func

Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

LGTM

@cjacek cjacek merged commit a9d9483 into llvm:main Nov 15, 2024
10 checks passed
@cjacek cjacek deleted the lib-invalid-name branch November 15, 2024 15:14
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.

4 participants