[Clang][Cygwin] Link default-manifest.o if exists

[kikairoya]

The Cygwin distribution provides an optional package to embed a manifest xml to disable the UAC escalation dialog.
Link automatically the manifest only if it exists when linking an executable (but not for DLLs, i.e., -mdll or -shared).
This matches GCC's behavior.

[llvmorg-github-actions]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Tomohiro Kashiwada (kikairoya)

Changes

The Cygwin distribution provides an optional package to embed a manifest xml to disable the UAC escalation dialog.
Link automatically the manifest only if it exists when linking an executable.
This matches GCC's behavior.

---

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

3 Files Affected:

• (modified) clang/lib/Driver/ToolChains/Cygwin.cpp (+7-1)
• (added) clang/test/Driver/Inputs/basic_cygwin_tree/usr/lib/default-manifest.o ()
• (modified) clang/test/Driver/cygwin.cpp (+10-4)

diff --git a/clang/lib/Driver/ToolChains/Cygwin.cpp b/clang/lib/Driver/ToolChains/Cygwin.cpp
index 938f6c3b5f2db..7d4167372af27 100644
--- a/clang/lib/Driver/ToolChains/Cygwin.cpp
+++ b/clang/lib/Driver/ToolChains/Cygwin.cpp
@@ -375,8 +375,14 @@ void cygwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
         tools::AddRunTimeLibs(ToolChain, D, CmdArgs, Args);
     }
 
-    if (!Args.hasArg(options::OPT_nostartfiles))
+    if (!Args.hasArg(options::OPT_nostartfiles)) {
+      if (!Args.hasArg(options::OPT_mdll, options::OPT_shared)) {
+        if (std::string O = ToolChain.GetFilePath("default-manifest.o");
+            O != "default-manifest.o")
+          CmdArgs.push_back(Args.MakeArgString(std::move(O)));
+      }
       CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crtend.o")));
+    }
   }
 
   Args.addAllArgs(CmdArgs, {options::OPT_T, options::OPT_t});
diff --git a/clang/test/Driver/Inputs/basic_cygwin_tree/usr/lib/default-manifest.o b/clang/test/Driver/Inputs/basic_cygwin_tree/usr/lib/default-manifest.o
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/cygwin.cpp b/clang/test/Driver/cygwin.cpp
index c51a359bb35e2..66fa9fbcfc1e0 100644
--- a/clang/test/Driver/cygwin.cpp
+++ b/clang/test/Driver/cygwin.cpp
@@ -20,6 +20,7 @@
 // CHECK-SAME: "-m" "i386pe"
 // CHECK-SAME: "{{.*}}{{/|\\\\}}crt0.o"
 // CHECK-SAME: "{{.*}}i686-pc-cygwin{{/|\\\\}}{{[0-9.]*}}{{/|\\\\}}crtbegin.o"
+// CHECK-SAME: "[[SYSROOT]]{{/|\\\\}}usr{{/|\\\\}}lib{{/|\\\\}}default-manifest.o"
 // CHECK-SAME: "{{.*}}i686-pc-cygwin{{/|\\\\}}{{[0-9.]*}}{{/|\\\\}}crtend.o"
 
 // RUN: %clang -### %s --target=i686-pc-cygwin --sysroot=%S/Inputs/basic_cygwin_tree \
@@ -30,7 +31,7 @@
 // CHECK-STATIC-SAME: "-Bstatic"
 
 // RUN: %clang -### %s --target=i686-pc-cygwin --sysroot=%S/Inputs/basic_cygwin_tree \
-// RUN:   -shared 2>&1 | FileCheck --check-prefix=CHECK-SHARED %s
+// RUN:   -shared 2>&1 | FileCheck --check-prefix=CHECK-SHARED %s --implicit-check-not=default-manifest
 // CHECK-SHARED:      "{{.*}}ld{{(\.exe)?}}"
 // CHECK-SHARED-SAME: "--shared"
 // CHECK-SHARED-SAME: "-e" "__cygwin_dll_entry@12"
@@ -67,6 +68,7 @@
 // CHECK-64-SAME: "-m" "i386pep"
 // CHECK-64-SAME: "{{.*}}{{/|\\\\}}crt0.o"
 // CHECK-64-SAME: "{{.*}}x86_64-pc-msys{{/|\\\\}}{{[0-9.]*}}{{/|\\\\}}crtbegin.o"
+// CHECK-64-SAME: "[[SYSROOT]]{{/|\\\\}}usr{{/|\\\\}}lib{{/|\\\\}}default-manifest.o"
 // CHECK-64-SAME: "{{.*}}x86_64-pc-msys{{/|\\\\}}{{[0-9.]*}}{{/|\\\\}}crtend.o"
 
 // RUN: %clang -### %s --target=x86_64-pc-cygwin --sysroot=%S/Inputs/basic_cygwin_tree \
@@ -77,7 +79,7 @@
 // CHECK-64-STATIC-SAME: "-Bstatic"
 
 // RUN: %clang -### %s --target=x86_64-pc-cygwin --sysroot=%S/Inputs/basic_cygwin_tree \
-// RUN:   -shared 2>&1 | FileCheck --check-prefix=CHECK-64-SHARED %s
+// RUN:   -shared 2>&1 | FileCheck --check-prefix=CHECK-64-SHARED %s --implicit-check-not=default-manifest
 // CHECK-64-SHARED:      "{{.*}}ld{{(\.exe)?}}"
 // CHECK-64-SHARED-SAME: "--shared"
 // CHECK-64-SHARED-SAME: "-e" "_cygwin_dll_entry"
@@ -94,13 +96,13 @@
 // CHECK-64-CROSS-SAME: "{{.*}}x86_64-pc-cygwin{{/|\\\\}}{{[0-9.]*}}{{/|\\\\}}crtend.o"
 
 // RUN: %clang -### %s --target=x86_64-pc-cygwin --sysroot=%S/Inputs/basic_cygwin_tree \
-// RUN:   -mdll 2>&1 | FileCheck --check-prefix=CHECK-64-DLL %s
+// RUN:   -mdll 2>&1 | FileCheck --check-prefix=CHECK-64-DLL %s --implicit-check-not=default-manifest
 // CHECK-64-DLL:      "{{.*}}ld{{(\.exe)?}}"
 // CHECK-64-DLL-SAME: "--dll"
 // CHECK-64-DLL-SAME: "-e" "_cygwin_dll_entry"
 
 // RUN: %clang -### %s --target=i686-pc-cygwin --sysroot=%S/Inputs/basic_cygwin_tree \
-// RUN:   -mdll 2>&1 | FileCheck --check-prefix=CHECK-DLL %s
+// RUN:   -mdll 2>&1 | FileCheck --check-prefix=CHECK-DLL %s --implicit-check-not=default-manifest
 // CHECK-DLL:      "{{.*}}ld{{(\.exe)?}}"
 // CHECK-DLL-SAME: "--dll"
 // CHECK-DLL-SAME: "-e" "__cygwin_dll_entry@12"
@@ -176,3 +178,7 @@
 // RUN:   2>&1 | FileCheck --check-prefix=CHECK-64-RTLIB-PER-TARGET %s
 // CHECK-64-RTLIB-PER-TARGET:      "{{.*}}ld{{(\.exe)?}}"
 // CHECK-64-RTLIB-PER-TARGET-SAME: "{{.*}}{{/|\\\\}}lib{{/|\\\\}}x86_64-pc-windows-cygnus{{/|\\\\}}libclang_rt.builtins.a"
+
+// RUN: %clang -### %s --target=i686-pc-cygwin --sysroot=%S/Inputs/basic_cross_cygwin_tree \
+// RUN:   2>&1 | FileCheck --check-prefix=CHECK-MANIFEST-ABSENT %s
+// CHECK-MANIFEST-ABSENT-NOT: default-manifest

[mstorsjo]

FWIW, GCC also looks for a similar file in mingw contexts, which Clang also doesn't use. It would probably be good for consistency to have Clang link that as well, but there hasn't been that many requests for it, IIRC.

(CC @lazka @lhmouse)

[mstorsjo]

Using a separate variable declaration in the if statement (if (type var = init; expression)) seems like an unusual style - I don't think it would hurt readability to just define the variable outside of the if in this case?

[kikairoya]

FWIW, GCC also looks for a similar file in mingw contexts, which Clang also doesn't use. It would probably be good for consistency to have Clang link that as well, but there hasn't been that many requests for it, IIRC.

Lacking the manifest causes some failures of tests due to their file name. (UAC is usually disabled in CI environments, so it doesn't matter there.)

[kikairoya]

If we want to do the same for MinGW, I'm happy to make a PR for that.

[mstorsjo]

If we want to do the same for MinGW, I'm happy to make a PR for that.

I think that might be useful, or what do @lazka and @lhmouse think?

[mstorsjo]

LGTM, thanks!

[lazka]

If we want to do the same for MinGW, I'm happy to make a PR for that.

I think that might be useful, or what do @lazka and @lhmouse think?

Yes, I think it would be helpful. Reducing the diff to gcc, and providing a better out of the box experience without having to deal with manifests. For context, this is what is currently used: https://sourceware.org/git/?p=cygwin-apps/windows-default-manifest.git;a=blob;f=default-manifest.rc;h=2c4ddf4783b953afaf7df9473450bf95842d1911;hb=HEAD

[lhmouse]

Sounds good to me, too.

[kikairoya]

It feels like it would be nice to have a helper, like ToolChain.GetFileIfExists(), which e.g. would return an std::optionalstd::string, which would make the conditional clearer instead of having to repeat the file name.

I'll submit a follow-up PR, with fixing other similar constructs as well.

submitted. #221063