@llvm/pr-subscribers-clang-driver

Author: Fangrui Song (MaskRay)

diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index ad68c086b71790..9d6ea371f9f6dd 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -39,8 +39,6 @@ static const SanitizerMask NotAllowedWithTrap = SanitizerKind::Vptr;
 static const SanitizerMask NotAllowedWithMinimalRuntime = SanitizerKind::Vptr;
 static const SanitizerMask NotAllowedWithExecuteOnly =
     SanitizerKind::Function | SanitizerKind::KCFI;
-static const SanitizerMask RequiresPIE =
-    SanitizerKind::DataFlow | SanitizerKind::Scudo;
 static const SanitizerMask NeedsUnwindTables =
     SanitizerKind::Address | SanitizerKind::HWAddress | SanitizerKind::Thread |
     SanitizerKind::Memory | SanitizerKind::DataFlow;
@@ -303,9 +301,7 @@ bool SanitizerArgs::needsCfiDiagRt() const {
          CfiCrossDso && !ImplicitCfiRuntime;
 }
 
-bool SanitizerArgs::requiresPIE() const {
-  return NeedPIE || (Sanitizers.Mask & RequiresPIE);
-}
+bool SanitizerArgs::requiresPIE() const { return NeedPIE; }
 
 bool SanitizerArgs::needsUnwindTables() const {
   return static_cast<bool>(Sanitizers.Mask & NeedsUnwindTables);
@@ -699,8 +695,6 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
     MsanParamRetval = Args.hasFlag(
         options::OPT_fsanitize_memory_param_retval,
         options::OPT_fno_sanitize_memory_param_retval, MsanParamRetval);
-    NeedPIE |= !(TC.getTriple().isOSLinux() &&
-                 TC.getTriple().getArch() == llvm::Triple::x86_64);
   } else if (AllAddedKinds & SanitizerKind::KernelMemory) {
     MsanUseAfterDtor = false;
     MsanParamRetval = Args.hasFlag(

@llvm/pr-subscribers-clang

Author: Fangrui Song (MaskRay)

diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index ad68c086b71790..9d6ea371f9f6dd 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -39,8 +39,6 @@ static const SanitizerMask NotAllowedWithTrap = SanitizerKind::Vptr;
 static const SanitizerMask NotAllowedWithMinimalRuntime = SanitizerKind::Vptr;
 static const SanitizerMask NotAllowedWithExecuteOnly =
     SanitizerKind::Function | SanitizerKind::KCFI;
-static const SanitizerMask RequiresPIE =
-    SanitizerKind::DataFlow | SanitizerKind::Scudo;
 static const SanitizerMask NeedsUnwindTables =
     SanitizerKind::Address | SanitizerKind::HWAddress | SanitizerKind::Thread |
     SanitizerKind::Memory | SanitizerKind::DataFlow;
@@ -303,9 +301,7 @@ bool SanitizerArgs::needsCfiDiagRt() const {
          CfiCrossDso && !ImplicitCfiRuntime;
 }
 
-bool SanitizerArgs::requiresPIE() const {
-  return NeedPIE || (Sanitizers.Mask & RequiresPIE);
-}
+bool SanitizerArgs::requiresPIE() const { return NeedPIE; }
 
 bool SanitizerArgs::needsUnwindTables() const {
   return static_cast<bool>(Sanitizers.Mask & NeedsUnwindTables);
@@ -699,8 +695,6 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
     MsanParamRetval = Args.hasFlag(
         options::OPT_fsanitize_memory_param_retval,
         options::OPT_fno_sanitize_memory_param_retval, MsanParamRetval);
-    NeedPIE |= !(TC.getTriple().isOSLinux() &&
-                 TC.getTriple().getArch() == llvm::Triple::x86_64);
   } else if (AllAddedKinds & SanitizerKind::KernelMemory) {
     MsanUseAfterDtor = false;
     MsanParamRetval = Args.hasFlag(

@MaskRay I see that in 3bbc912 you removed some tests that fail because of this change. Why do you think that is an appropriate solution? I have some other tests in a downstream product that are failing because we build with CLANG_DEFAULT_PIE_ON_LINUX=OFF. You said in your initial comment here that "most builds set CLANG_DEFAULT_PIE_ON_LINUX to 1, making RequiresPIE/NeedPIE redundant on Linux." But apparently it's not redundant for builds that don't use that setting.

Do you have another solution in progress (or already committed that I haven't seen yet)? It seems that as long as this is a configurable option, we need to support both settings. Intel's SYCL project (https://github.com/intel/llvm) currently sets CLANG_DEFAULT_PIE_ON_LINUX to zero for compatibility with gcc in Fedora releases (at least, I think that's the reason).

@MaskRay I see that in 3bbc912 you removed some tests that fail because of this change. Why do you think that is an appropriate solution? I have some other tests in a downstream product that are failing because we build with CLANG_DEFAULT_PIE_ON_LINUX=OFF. You said in your initial comment here that "most builds set CLANG_DEFAULT_PIE_ON_LINUX to 1, making RequiresPIE/NeedPIE redundant on Linux." But apparently it's not redundant for builds that don't use that setting.

Do you have another solution in progress (or already committed that I haven't seen yet)? It seems that as long as this is a configurable option, we need to support both settings. Intel's SYCL project (intel/llvm) currently sets CLANG_DEFAULT_PIE_ON_LINUX to zero for compatibility with gcc in Fedora releases (at least, I think that's the reason).

My internal users also use CLANG_DEFAULT_PIE_ON_LINUX OFF, so I definitely want to support both flavors.

The RUN lines removed by 3bbc912 no longer made sense (the commit message could have been reworded). They wanted to check that we defaulted to -fPIE even when no -fno-pic/-fpie/-fpic was specified. The force-PIC effect might be a previous limitation, or possibly just something cargo culted from the previous msan limitation.

If I use scudo_flags = ["-fsanitize=scudo", "-fno-pic", "-no-pie"] in test/scudo/lit.cfg.py, check-scudo_standalone still passes.

@MaskRay I see that in 3bbc912 you removed some tests that fail because of this change. Why do you think that is an appropriate solution? I have some other tests in a downstream product that are failing because we build with CLANG_DEFAULT_PIE_ON_LINUX=OFF. You said in your initial comment here that "most builds set CLANG_DEFAULT_PIE_ON_LINUX to 1, making RequiresPIE/NeedPIE redundant on Linux." But apparently it's not redundant for builds that don't use that setting.
Do you have another solution in progress (or already committed that I haven't seen yet)? It seems that as long as this is a configurable option, we need to support both settings. Intel's SYCL project (intel/llvm) currently sets CLANG_DEFAULT_PIE_ON_LINUX to zero for compatibility with gcc in Fedora releases (at least, I think that's the reason).

My internal users also use CLANG_DEFAULT_PIE_ON_LINUX OFF, so I definitely want to support both flavors.

The RUN lines removed by 3bbc912 no longer made sense (the commit message could have been reworded). They wanted to check that we defaulted to -fPIE even when no -fno-pic/-fpie/-fpic was specified. The force-PIC effect might be a previous limitation, or possibly just something cargo culted from the previous msan limitation.

If I use scudo_flags = ["-fsanitize=scudo", "-fno-pic", "-no-pie"] in test/scudo/lit.cfg.py, check-scudo_standalone still passes.

I see. The change to the tests makes sense with that explanation.

The test we were seeing fail is compiler-rt/test/dfsan/custom.cpp, and I'm told it fails with the main LLVM project if CLANG_DEFAULT_PIE_ON_LINUX=OFF is used. I guess we don't have a buildbot that runs that particular test with that setting? The failure is a segmentation fault in dfsan/X86_64Config/Output/custom.cpp.script. I'm not familiar enough with the dfsan tests to say what this means, but it is triggered by the change in this PR.

@MaskRay I see that in 3bbc912 you removed some tests that fail because of this change. Why do you think that is an appropriate solution? I have some other tests in a downstream product that are failing because we build with CLANG_DEFAULT_PIE_ON_LINUX=OFF. You said in your initial comment here that "most builds set CLANG_DEFAULT_PIE_ON_LINUX to 1, making RequiresPIE/NeedPIE redundant on Linux." But apparently it's not redundant for builds that don't use that setting.
Do you have another solution in progress (or already committed that I haven't seen yet)? It seems that as long as this is a configurable option, we need to support both settings. Intel's SYCL project (intel/llvm) currently sets CLANG_DEFAULT_PIE_ON_LINUX to zero for compatibility with gcc in Fedora releases (at least, I think that's the reason).

My internal users also use CLANG_DEFAULT_PIE_ON_LINUX OFF, so I definitely want to support both flavors.
The RUN lines removed by 3bbc912 no longer made sense (the commit message could have been reworded). They wanted to check that we defaulted to -fPIE even when no -fno-pic/-fpie/-fpic was specified. The force-PIC effect might be a previous limitation, or possibly just something cargo culted from the previous msan limitation.
If I use scudo_flags = ["-fsanitize=scudo", "-fno-pic", "-no-pie"] in test/scudo/lit.cfg.py, check-scudo_standalone still passes.

I see. The change to the tests makes sense with that explanation.

The test we were seeing fail is compiler-rt/test/dfsan/custom.cpp, and I'm told it fails with the main LLVM project if CLANG_DEFAULT_PIE_ON_LINUX=OFF is used. I guess we don't have a buildbot that runs that particular test with that setting? The failure is a segmentation fault in dfsan/X86_64Config/Output/custom.cpp.script. I'm not familiar enough with the dfsan tests to say what this means, but it is triggered by the change in this PR.

Thanks for the report. I noticed this error internally as well. There were two portability issues in dfsan/custom.cpp, fixed by #78363 and 8434e5d .

I've changed one RUN line to -no-pie to test ET_EXEC executables.

There is no CLANG_DEFAULT_PIE_ON_LINUX=off bot but I think the coverage is sufficient.