Skip to content
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

release/19.x: [LLD] [MinGW] Recognize the -rpath option (#102886) #104843

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Aug 19, 2024

Backport 69f76c7

Requested by: @mstorsjo

@llvmbot llvmbot added this to the LLVM 19.X Release milestone Aug 19, 2024
@llvmbot
Copy link
Collaborator Author

llvmbot commented Aug 19, 2024

@alvinhochun What do you think about merging this PR to the release branch?

@llvmbot
Copy link
Collaborator Author

llvmbot commented Aug 19, 2024

@llvm/pr-subscribers-lld

Author: None (llvmbot)

Changes

Backport 69f76c7

Requested by: @mstorsjo


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

3 Files Affected:

  • (modified) lld/MinGW/Driver.cpp (+3)
  • (modified) lld/MinGW/Options.td (+3)
  • (modified) lld/test/MinGW/driver.test (+6)
diff --git a/lld/MinGW/Driver.cpp b/lld/MinGW/Driver.cpp
index 35fd478a21905e..c7d7b9cfca386f 100644
--- a/lld/MinGW/Driver.cpp
+++ b/lld/MinGW/Driver.cpp
@@ -448,6 +448,9 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
       add("-errorlimit:" + s);
   }
 
+  if (auto *a = args.getLastArg(OPT_rpath))
+    warn("parameter " + a->getSpelling() + " has no effect on PE/COFF targets");
+
   for (auto *a : args.filtered(OPT_mllvm))
     add("-mllvm:" + StringRef(a->getValue()));
 
diff --git a/lld/MinGW/Options.td b/lld/MinGW/Options.td
index 56f67e3dd96c42..7bd5fb80749da2 100644
--- a/lld/MinGW/Options.td
+++ b/lld/MinGW/Options.td
@@ -243,6 +243,9 @@ defm: EqNoHelp<"sysroot">;
 def: F<"sort-common">;
 def: F<"start-group">;
 
+// Ignored options, that produce warnings
+defm rpath: EqNoHelp<"rpath">;
+
 // Ignore GCC collect2 LTO plugin related options. Note that we don't support
 // GCC LTO, but GCC collect2 passes these options even in non-LTO mode.
 def: J<"plugin-opt=-fresolution=">;
diff --git a/lld/test/MinGW/driver.test b/lld/test/MinGW/driver.test
index cbccd09793c2f0..0dab66b613c774 100644
--- a/lld/test/MinGW/driver.test
+++ b/lld/test/MinGW/driver.test
@@ -446,3 +446,9 @@ RUN: ld.lld -### foo.o -m i386pep --build-id=none 2>&1 | FileCheck -check-prefix
 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
 NO_BUILD_ID: -build-id:no
+
+RUN: ld.lld -### foo.o -m i386pep -rpath foo 2>&1 | FileCheck -check-prefix=WARN_RPATH %s
+RUN: ld.lld -### foo.o -m i386pep --rpath foo 2>&1 | FileCheck -check-prefix=WARN_RPATH %s
+RUN: ld.lld -### foo.o -m i386pep -rpath=foo 2>&1 | FileCheck -check-prefix=WARN_RPATH %s
+RUN: ld.lld -### foo.o -m i386pep --rpath=foo 2>&1 | FileCheck -check-prefix=WARN_RPATH %s
+WARN_RPATH: warning: parameter -{{-?}}rpath has no effect on PE/COFF targets

@mstorsjo
Copy link
Member

@alvinhochun What do you think about merging this PR to the release branch?

I think @cjacek or @MaskRay could comment as well.

Technically, this isn't indeed a fix for any regression, but it's a quite safe fix for improving drop-in compatibility vs GNU tools.

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 commit itself seems quite simple and seems safe on its own. I guess the main worry would be that some build system configure checks may, in theory, now consider it as available and start using a different, potentially unintended, code paths. Still, given that it was the case with GNU tools, that risk seems small.

@mstorsjo
Copy link
Member

The commit itself seems quite simple and seems safe on its own. I guess the main worry would be that some build system configure checks may, in theory, now consider it as available and start using a different, potentially unintended, code paths. Still, given that it was the case with GNU tools, that risk seems small.

Yes, I guess in theory, you could have a build system that checks whether -rpath is accepted, and starts adding it if it is. Such cases will now start getting warnings... I'm not aware of any such case though, but it's theoretically possible at least.

GNU ld silently accepts the -rpath option for Windows targets, as a
no-op.

This has lead to some build systems (and users) passing this option
while building for Windows/MinGW, even if Windows doesn't have any
concept like rpath.

Older versions of Conan did include -rpath in the pkg-config files it
generated, see e.g.

https://github.com/conan-io/conan/blob/17c58f0c61931f9de218ac571cd97a8e0befa68e/conans/client/generators/pkg_config.py#L104-L114
and
https://github.com/conan-io/conan/blob/17c58f0c61931f9de218ac571cd97a8e0befa68e/conans/client/build/compiler_flags.py#L26-L34
- and see mstorsjo/llvm-mingw#300 for user
reports about this issue.

Recognize the option in LLD for MinGW targets, to improve drop-in
compatibility compared to GNU ld, but produce a warning to alert users
that the option really has no effect for these targets.

(cherry picked from commit 69f76c7)
@tru tru merged commit 6dbc0e2 into llvm:release/19.x Aug 20, 2024
7 of 9 checks passed
Copy link

@mstorsjo (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

@mstorsjo
Copy link
Member

Release note:
LLD now recognizes the -rpath option in MinGW mode (instead of erroring out). The option is a no-op, and LLD warns about it when passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants