[Clang][MinGW] Pass --functionpadmin to the linker when -fms-hotpatch is used #116512

cjacek · 2024-11-16T22:17:29Z

No description provided.

This introduces the MinGW counterpart of lld-link's -functionpadmin.

… is used

llvmbot · 2024-11-16T22:18:01Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-lld

Author: Jacek Caban (cjacek)

Changes

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

5 Files Affected:

(modified) clang/lib/Driver/ToolChains/MinGW.cpp (+3)
(modified) clang/test/Driver/mingw.cpp (+4)
(modified) lld/MinGW/Driver.cpp (+8)
(modified) lld/MinGW/Options.td (+3)
(modified) lld/test/MinGW/driver.test (+7)

diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp b/clang/lib/Driver/ToolChains/MinGW.cpp
index e51daca5025a80..963de81027ca9f 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -188,6 +188,9 @@ void tools::MinGW::Linker::ConstructJob(Compilation &C, const JobAction &JA,
           << A->getSpelling() << GuardArgs;
   }
 
+  if (Args.hasArg(options::OPT_fms_hotpatch))
+    CmdArgs.push_back("--functionpadmin");
+
   CmdArgs.push_back("-o");
   const char *OutputFile = Output.getFilename();
   // GCC implicitly adds an .exe extension if it is given an output file name
diff --git a/clang/test/Driver/mingw.cpp b/clang/test/Driver/mingw.cpp
index 4a9ba4d259b46f..9790c86a364f85 100644
--- a/clang/test/Driver/mingw.cpp
+++ b/clang/test/Driver/mingw.cpp
@@ -84,3 +84,7 @@
 // RUN: %clang --target=arm64ec-windows-gnu -### -o /dev/null %s 2>&1 \
 // RUN:   | FileCheck %s --check-prefix CHECK_MINGW_EC_LINK
 // CHECK_MINGW_EC_LINK: "-m" "arm64ecpe"
+
+// RUN: %clang --target=i686-windows-gnu -fms-hotpatch -### -- %s 2>&1 \
+// RUN:    | FileCheck %s --check-prefix=FUNCTIONPADMIN
+// FUNCTIONPADMIN: "--functionpadmin"
diff --git a/lld/MinGW/Driver.cpp b/lld/MinGW/Driver.cpp
index 553698d4f537fc..706687202b19fb 100644
--- a/lld/MinGW/Driver.cpp
+++ b/lld/MinGW/Driver.cpp
@@ -329,6 +329,14 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
       add("-build-id");
   }
 
+  if (auto *a = args.getLastArg(OPT_functionpadmin)) {
+    StringRef v = a->getValue();
+    if (v.empty())
+      add("-functionpadmin");
+    else
+      add("-functionpadmin:" + v);
+  }
+
   if (args.hasFlag(OPT_fatal_warnings, OPT_no_fatal_warnings, false))
     add("-WX");
   else
diff --git a/lld/MinGW/Options.td b/lld/MinGW/Options.td
index ff7e21fa808f39..abe5e5c82ca3ff 100644
--- a/lld/MinGW/Options.td
+++ b/lld/MinGW/Options.td
@@ -215,6 +215,9 @@ defm error_limit:
 def build_id: J<"build-id=">, HelpText<"Generate build ID note (pass none to disable)">, 
   MetaVarName<"<arg>">;
 def : F<"build-id">, Alias<build_id>, HelpText<"Alias for --build-id=">;
+def functionpadmin: J<"functionpadmin=">, HelpText<"Prepares an image for hotpatching">,
+  MetaVarName<"<arg>">;
+def : F<"functionpadmin">, Alias<functionpadmin>, HelpText<"Alias for --functionpadmin=">;
 
 // Alias
 def alias_Bdynamic_call_shared: Flag<["-"], "call_shared">, Alias<Bdynamic>;
diff --git a/lld/test/MinGW/driver.test b/lld/test/MinGW/driver.test
index 2831d155fef128..ed6bc880f8f2d9 100644
--- a/lld/test/MinGW/driver.test
+++ b/lld/test/MinGW/driver.test
@@ -445,6 +445,13 @@ RUN: ld.lld -### foo.o -m i386pep --build-id=fast 2>&1 | FileCheck -check-prefix
 BUILD_ID_WARN: unsupported build id hashing: fast, using default hashing.
 BUILD_ID_WARN: -build-id{{ }}
 
+RUN: ld.lld -### foo.o -m i386pep --functionpadmin= 2>&1 | FileCheck -check-prefix=FUNCTIONPADMIN %s
+RUN: ld.lld -### foo.o -m i386pep --functionpadmin 2>&1 | FileCheck -check-prefix=FUNCTIONPADMIN %s
+FUNCTIONPADMIN: -functionpadmin{{ }}
+
+RUN: ld.lld -### foo.o -m i386pep --functionpadmin=2 2>&1 | FileCheck -check-prefix=FUNCTIONPADMIN2 %s
+FUNCTIONPADMIN2: -functionpadmin:2{{ }}
+
 RUN: ld.lld -### foo.o -m i386pep --build-id=none 2>&1 | FileCheck -check-prefix=NO_BUILD_ID %s
 RUN: ld.lld -### foo.o -m i386pep -s 2>&1 | FileCheck -check-prefix=NO_BUILD_ID %s
 RUN: ld.lld -### foo.o -m i386pep -S 2>&1 | FileCheck -check-prefix=NO_BUILD_ID %s

llvmbot · 2024-11-16T22:18:02Z

@llvm/pr-subscribers-clang-driver

Author: Jacek Caban (cjacek)

Changes

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

5 Files Affected:

(modified) clang/lib/Driver/ToolChains/MinGW.cpp (+3)
(modified) clang/test/Driver/mingw.cpp (+4)
(modified) lld/MinGW/Driver.cpp (+8)
(modified) lld/MinGW/Options.td (+3)
(modified) lld/test/MinGW/driver.test (+7)

diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp b/clang/lib/Driver/ToolChains/MinGW.cpp
index e51daca5025a80..963de81027ca9f 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -188,6 +188,9 @@ void tools::MinGW::Linker::ConstructJob(Compilation &C, const JobAction &JA,
           << A->getSpelling() << GuardArgs;
   }
 
+  if (Args.hasArg(options::OPT_fms_hotpatch))
+    CmdArgs.push_back("--functionpadmin");
+
   CmdArgs.push_back("-o");
   const char *OutputFile = Output.getFilename();
   // GCC implicitly adds an .exe extension if it is given an output file name
diff --git a/clang/test/Driver/mingw.cpp b/clang/test/Driver/mingw.cpp
index 4a9ba4d259b46f..9790c86a364f85 100644
--- a/clang/test/Driver/mingw.cpp
+++ b/clang/test/Driver/mingw.cpp
@@ -84,3 +84,7 @@
 // RUN: %clang --target=arm64ec-windows-gnu -### -o /dev/null %s 2>&1 \
 // RUN:   | FileCheck %s --check-prefix CHECK_MINGW_EC_LINK
 // CHECK_MINGW_EC_LINK: "-m" "arm64ecpe"
+
+// RUN: %clang --target=i686-windows-gnu -fms-hotpatch -### -- %s 2>&1 \
+// RUN:    | FileCheck %s --check-prefix=FUNCTIONPADMIN
+// FUNCTIONPADMIN: "--functionpadmin"
diff --git a/lld/MinGW/Driver.cpp b/lld/MinGW/Driver.cpp
index 553698d4f537fc..706687202b19fb 100644
--- a/lld/MinGW/Driver.cpp
+++ b/lld/MinGW/Driver.cpp
@@ -329,6 +329,14 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
       add("-build-id");
   }
 
+  if (auto *a = args.getLastArg(OPT_functionpadmin)) {
+    StringRef v = a->getValue();
+    if (v.empty())
+      add("-functionpadmin");
+    else
+      add("-functionpadmin:" + v);
+  }
+
   if (args.hasFlag(OPT_fatal_warnings, OPT_no_fatal_warnings, false))
     add("-WX");
   else
diff --git a/lld/MinGW/Options.td b/lld/MinGW/Options.td
index ff7e21fa808f39..abe5e5c82ca3ff 100644
--- a/lld/MinGW/Options.td
+++ b/lld/MinGW/Options.td
@@ -215,6 +215,9 @@ defm error_limit:
 def build_id: J<"build-id=">, HelpText<"Generate build ID note (pass none to disable)">, 
   MetaVarName<"<arg>">;
 def : F<"build-id">, Alias<build_id>, HelpText<"Alias for --build-id=">;
+def functionpadmin: J<"functionpadmin=">, HelpText<"Prepares an image for hotpatching">,
+  MetaVarName<"<arg>">;
+def : F<"functionpadmin">, Alias<functionpadmin>, HelpText<"Alias for --functionpadmin=">;
 
 // Alias
 def alias_Bdynamic_call_shared: Flag<["-"], "call_shared">, Alias<Bdynamic>;
diff --git a/lld/test/MinGW/driver.test b/lld/test/MinGW/driver.test
index 2831d155fef128..ed6bc880f8f2d9 100644
--- a/lld/test/MinGW/driver.test
+++ b/lld/test/MinGW/driver.test
@@ -445,6 +445,13 @@ RUN: ld.lld -### foo.o -m i386pep --build-id=fast 2>&1 | FileCheck -check-prefix
 BUILD_ID_WARN: unsupported build id hashing: fast, using default hashing.
 BUILD_ID_WARN: -build-id{{ }}
 
+RUN: ld.lld -### foo.o -m i386pep --functionpadmin= 2>&1 | FileCheck -check-prefix=FUNCTIONPADMIN %s
+RUN: ld.lld -### foo.o -m i386pep --functionpadmin 2>&1 | FileCheck -check-prefix=FUNCTIONPADMIN %s
+FUNCTIONPADMIN: -functionpadmin{{ }}
+
+RUN: ld.lld -### foo.o -m i386pep --functionpadmin=2 2>&1 | FileCheck -check-prefix=FUNCTIONPADMIN2 %s
+FUNCTIONPADMIN2: -functionpadmin:2{{ }}
+
 RUN: ld.lld -### foo.o -m i386pep --build-id=none 2>&1 | FileCheck -check-prefix=NO_BUILD_ID %s
 RUN: ld.lld -### foo.o -m i386pep -s 2>&1 | FileCheck -check-prefix=NO_BUILD_ID %s
 RUN: ld.lld -### foo.o -m i386pep -S 2>&1 | FileCheck -check-prefix=NO_BUILD_ID %s

cjacek · 2024-11-16T22:25:04Z

This PR depends on #116511.

It updates -fms-hotpatch to behave similarly during linking as it does in MSVC mode. However, one limitation remains: object files are marked as hotpatchable through CodeView data, so this is only effective when -gcodeview is enabled. This is similar to the behavior of Clang in MSVC mode when debug info is not enabled. MSVC seems to always emit S_COMPILE3, even when debug info is disabled (in which case, it does not emit any other debug information).

mstorsjo

This looks reasonable.

Is this something that GCC doesn't have? IIRC they do have some support for hotpatchability in some way - can you briefly summarize what they have and what we have in common and what differs?

cjacek · 2024-11-16T22:42:58Z

GCC offers the ms_hook_prologue function attribute as an alternative. It provides similar guarantees: it inserts a hardcoded prologue and adds padding. However, it applies on a per-function basis, so there isn’t an easy way to make the entire module hotpatchable. In GCC, the padding is handled directly by the compiler, with no linker involvement, which emits assembly like:

        .globl  _func
        .def    _func;  .scl    2;      .type   32;     .endef
        .long    0xcccccccc
        .long    0xcccccccc
        .long    0xcccccccc
        .long    0xcccccccc
_func:
        .byte   0x8b, 0xff, 0x55, 0x8b, 0xec
        movl    $1, %eax
        popl    %ebp
        ret

cjacek added 2 commits November 16, 2024 23:12

[LLD][MinGW] Add support for --functionpadmin option

c13f631

This introduces the MinGW counterpart of lld-link's -functionpadmin.

[clang][MinGW] Pass --functionpadmin to the linker when -fms-hotpatch…

51ebafd

… is used

cjacek requested a review from mstorsjo November 16, 2024 22:17

llvmbot added clang Clang issues not falling into any other category lld clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Nov 16, 2024

mstorsjo approved these changes Nov 16, 2024

View reviewed changes

cjacek merged commit 40ea61b into llvm:main Nov 18, 2024
12 checks passed

cjacek deleted the clang-hotpatch branch November 18, 2024 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][MinGW] Pass --functionpadmin to the linker when -fms-hotpatch is used #116512

[Clang][MinGW] Pass --functionpadmin to the linker when -fms-hotpatch is used #116512

Uh oh!

cjacek commented Nov 16, 2024

Uh oh!

llvmbot commented Nov 16, 2024 •

edited

Loading

Uh oh!

llvmbot commented Nov 16, 2024

Uh oh!

cjacek commented Nov 16, 2024

Uh oh!

mstorsjo left a comment

Uh oh!

cjacek commented Nov 16, 2024

Uh oh!

Uh oh!

Uh oh!

[Clang][MinGW] Pass --functionpadmin to the linker when -fms-hotpatch is used #116512

[Clang][MinGW] Pass --functionpadmin to the linker when -fms-hotpatch is used #116512

Uh oh!

Conversation

cjacek commented Nov 16, 2024

Uh oh!

llvmbot commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 16, 2024

Uh oh!

cjacek commented Nov 16, 2024

Uh oh!

mstorsjo left a comment

Choose a reason for hiding this comment

Uh oh!

cjacek commented Nov 16, 2024

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Nov 16, 2024 •

edited

Loading