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

[Driver] Make ELF -nopie specific to OpenBSD #72578

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Nov 16, 2023

-no-pie1/-nopie is rarely used and among the rare uses almost
everwhere uses -no-pie, since GCC does not recognize -nopie.
However, OpenBSD seems to use -nopie. Therefore, make -nopie specific
to OpenBSD to prevent newer ToolChains (Solaris, SerenityOS) from cargo
culting and copying -nopie.

@MaskRay MaskRay requested review from brad0 and rorth November 16, 2023 22:23
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Nov 16, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 16, 2023

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Fangrui Song (MaskRay)

Changes

-no-pie1/-nopie is rarely used and among the rare uses almost
everwhere uses -no-pie, since GCC does not recognize -nopie.
However, OpenBSD seems to prefer -nopie. Therefore, make -nopie specific
to OpenBSD to prevent newer ToolChains (Solaris, SerenityOS) from cargo
culting and copying -nopie.


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

6 Files Affected:

  • (modified) clang/include/clang/Driver/Options.td (+1-1)
  • (modified) clang/lib/Driver/ToolChains/Gnu.cpp (+3-7)
  • (modified) clang/lib/Driver/ToolChains/OpenBSD.cpp (+1-1)
  • (modified) clang/lib/Driver/ToolChains/Solaris.cpp (+2-5)
  • (modified) clang/test/Driver/android-pie.c (-2)
  • (modified) clang/test/Driver/pic.c (+3-3)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 811550416110b3d..654bf8be78d6259 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -5137,7 +5137,7 @@ def nofixprebinding : Flag<["-"], "nofixprebinding">;
 def nolibc : Flag<["-"], "nolibc">;
 def nomultidefs : Flag<["-"], "nomultidefs">;
 def nopie : Flag<["-"], "nopie">, Visibility<[ClangOption, FlangOption]>;
-def no_pie : Flag<["-"], "no-pie">, Visibility<[ClangOption, FlangOption]>, Alias<nopie>;
+def no_pie : Flag<["-"], "no-pie">, Visibility<[ClangOption, FlangOption]>;
 def noprebind : Flag<["-"], "noprebind">;
 def noprofilelib : Flag<["-"], "noprofilelib">;
 def noseglinkedit : Flag<["-"], "noseglinkedit">;
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp b/clang/lib/Driver/ToolChains/Gnu.cpp
index ba95ce9c5a28153..2b1e8f02cf66388 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -294,9 +294,7 @@ static const char *getLDMOption(const llvm::Triple &T, const ArgList &Args) {
 
 static bool getStaticPIE(const ArgList &Args, const ToolChain &TC) {
   bool HasStaticPIE = Args.hasArg(options::OPT_static_pie);
-  // -no-pie is an alias for -nopie. So, handling -nopie takes care of
-  // -no-pie as well.
-  if (HasStaticPIE && Args.hasArg(options::OPT_nopie)) {
+  if (HasStaticPIE && Args.hasArg(options::OPT_no_pie)) {
     const Driver &D = TC.getDriver();
     const llvm::opt::OptTable &Opts = D.getOpts();
     StringRef StaticPIEName = Opts.getOptionName(options::OPT_static_pie);
@@ -443,10 +441,8 @@ void tools::gnutools::Linker::ConstructJob(Compilation &C, const JobAction &JA,
     if (Args.hasArg(options::OPT_rdynamic))
       CmdArgs.push_back("-export-dynamic");
     if (!IsShared) {
-      Arg *A = Args.getLastArg(options::OPT_pie, options::OPT_no_pie,
-                               options::OPT_nopie);
-      IsPIE = A ? A->getOption().matches(options::OPT_pie)
-                : ToolChain.isPIEDefault(Args);
+      IsPIE = Args.hasFlag(options::OPT_pie, options::OPT_no_pie,
+                           ToolChain.isPIEDefault(Args));
       if (IsPIE)
         CmdArgs.push_back("-pie");
       CmdArgs.push_back("-dynamic-linker");
diff --git a/clang/lib/Driver/ToolChains/OpenBSD.cpp b/clang/lib/Driver/ToolChains/OpenBSD.cpp
index 5d06cd8ab0bad16..e883838e2dc9317 100644
--- a/clang/lib/Driver/ToolChains/OpenBSD.cpp
+++ b/clang/lib/Driver/ToolChains/OpenBSD.cpp
@@ -117,7 +117,7 @@ void openbsd::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   bool Shared = Args.hasArg(options::OPT_shared);
   bool Profiling = Args.hasArg(options::OPT_pg);
   bool Pie = Args.hasArg(options::OPT_pie);
-  bool Nopie = Args.hasArg(options::OPT_nopie);
+  bool Nopie = Args.hasArg(options::OPT_no_pie, options::OPT_nopie);
   const bool Relocatable = Args.hasArg(options::OPT_r);
 
   // Silence warning for "clang -g foo.o -o foo"
diff --git a/clang/lib/Driver/ToolChains/Solaris.cpp b/clang/lib/Driver/ToolChains/Solaris.cpp
index 96757f07a54974f..485730da7df1f8c 100644
--- a/clang/lib/Driver/ToolChains/Solaris.cpp
+++ b/clang/lib/Driver/ToolChains/Solaris.cpp
@@ -49,11 +49,8 @@ static bool getPIE(const ArgList &Args, const ToolChain &TC) {
       Args.hasArg(options::OPT_r))
     return false;
 
-  Arg *A = Args.getLastArg(options::OPT_pie, options::OPT_no_pie,
-                           options::OPT_nopie);
-  if (!A)
-    return TC.isPIEDefault(Args);
-  return A->getOption().matches(options::OPT_pie);
+  return Args.hasFlag(options::OPT_pie, options::OPT_no_pie,
+                      TC.isPIEDefault(Args));
 }
 
 // FIXME: Need to handle CLANG_DEFAULT_LINKER here?
diff --git a/clang/test/Driver/android-pie.c b/clang/test/Driver/android-pie.c
index 8620e185654586c..01720edf98fb170 100644
--- a/clang/test/Driver/android-pie.c
+++ b/clang/test/Driver/android-pie.c
@@ -36,8 +36,6 @@
 
 // RUN: %clang %s -### -o %t.o 2>&1 -no-pie --target=arm-linux-androideabi24 \
 // RUN:   | FileCheck --check-prefix=NO-PIE %s
-// RUN: %clang %s -### -o %t.o 2>&1 -nopie --target=arm-linux-androideabi24 \
-// RUN:   | FileCheck --check-prefix=NO-PIE %s
 // RUN: %clang %s -### -o %t.o 2>&1 -pie -no-pie --target=arm-linux-androideabi24 \
 // RUN:   | FileCheck --check-prefix=NO-PIE %s
 
diff --git a/clang/test/Driver/pic.c b/clang/test/Driver/pic.c
index 86b2a3b041d5f92..aeddead6dbf8f4c 100644
--- a/clang/test/Driver/pic.c
+++ b/clang/test/Driver/pic.c
@@ -166,11 +166,11 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-PIE2
 // RUN: %clang -c %s -target armv7-linux-musleabihf -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-PIE2
-// RUN: %clang %s -target x86_64-linux-musl -nopie -### 2>&1 \
+// RUN: %clang %s --target=x86_64-linux-musl -no-pie -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-NO-PIE
-// RUN: %clang %s -target x86_64-linux-musl -pie -nopie -### 2>&1 \
+// RUN: %clang %s --target=x86_64-linux-musl -pie -no-pie -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-NO-PIE
-// RUN: %clang %s -target x86_64-linux-musl -nopie -pie -### 2>&1 \
+// RUN: %clang %s --target=x86_64-linux-musl -no-pie -pie -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-PIE2
 //
 // Darwin is a beautiful and unique snowflake when it comes to these flags.

@brad0
Copy link
Contributor

brad0 commented Nov 16, 2023

-nopie is for the linker. We only use -fno-pie for the compiler.

@MaskRay
Copy link
Member Author

MaskRay commented Nov 17, 2023

-nopie is for the linker. We only use -fno-pie for the compiler.

OK. Then it seems that the driver option -nopie for linking should be removed even for OpenBSD?

@brad0
Copy link
Contributor

brad0 commented Nov 18, 2023

-nopie is for the linker. We only use -fno-pie for the compiler.

OK. Then it seems that the driver option -nopie for linking should be removed even for OpenBSD?

Let me check something before I say anything further.

@brad0
Copy link
Contributor

brad0 commented Nov 20, 2023

Continue with this as is.

-no-pie[1]/-nopie is rarely used and among the rare uses almost
everwhere uses -no-pie, since GCC does not recognize -nopie.
However, OpenBSD seems to use -nopie. Therefore, make -nopie specific
to OpenBSD to prevent newer ToolChains (Solaris, SerenityOS) from cargo
culting and copying -nopie.

[1]: https://reviews.llvm.org/D35462
@MaskRay
Copy link
Member Author

MaskRay commented Nov 21, 2023

Rebase to resolve a merge conflict. No change to the diff otherwise.

@MaskRay MaskRay merged commit cac82e2 into llvm:main Nov 21, 2023
2 of 3 checks passed
@MaskRay MaskRay deleted the drv-nopie branch November 21, 2023 07:15
@glandium
Copy link
Contributor

Weirdly, this leads to warning: argument unused during compilation: '-nopie' rather than error: unknown argument: '-nopie'

@MaskRay
Copy link
Member Author

MaskRay commented Nov 30, 2023

Weirdly, this leads to warning: argument unused during compilation: '-nopie' rather than error: unknown argument: '-nopie'

Marking -nopie as TargetSpecific should change this to an error.

https://github.com/openbsd/src has some -nopie use cases that haven't migrated.

MaskRay added a commit that referenced this pull request Nov 30, 2023
so that we get an `error: unsupported option '-nopie' for target ...` instead of a warning.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 1, 2023
…gesanspaille

Clang trunk removed its support in llvm/llvm-project#72578
and we were only using it because clang < 5 didn't support -no-pie. We
don't support those versions anymore, so we can use -no-pie
unconditionally now.

Differential Revision: https://phabricator.services.mozilla.com/D195061
aosmond pushed a commit to aosmond/gecko that referenced this pull request Dec 1, 2023
…gesanspaille

Clang trunk removed its support in llvm/llvm-project#72578
and we were only using it because clang < 5 didn't support -no-pie. We
don't support those versions anymore, so we can use -no-pie
unconditionally now.

Differential Revision: https://phabricator.services.mozilla.com/D195061
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Dec 7, 2023
…gesanspaille

Clang trunk removed its support in llvm/llvm-project#72578
and we were only using it because clang < 5 didn't support -no-pie. We
don't support those versions anymore, so we can use -no-pie
unconditionally now.

Differential Revision: https://phabricator.services.mozilla.com/D195061

UltraBlame original commit: 68ac06a41816bd364f1d413873d41e9cd5ef6606
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Dec 7, 2023
…gesanspaille

Clang trunk removed its support in llvm/llvm-project#72578
and we were only using it because clang < 5 didn't support -no-pie. We
don't support those versions anymore, so we can use -no-pie
unconditionally now.

Differential Revision: https://phabricator.services.mozilla.com/D195061

UltraBlame original commit: 68ac06a41816bd364f1d413873d41e9cd5ef6606
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Dec 7, 2023
…gesanspaille

Clang trunk removed its support in llvm/llvm-project#72578
and we were only using it because clang < 5 didn't support -no-pie. We
don't support those versions anymore, so we can use -no-pie
unconditionally now.

Differential Revision: https://phabricator.services.mozilla.com/D195061

UltraBlame original commit: 68ac06a41816bd364f1d413873d41e9cd5ef6606
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants