[PATCH] D158614: [UBSan] Disable the function sanitizer on an execute-only target.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:326
 def err_unsupported_abi_for_opt : Error<"'%0' can only be used with the '%1' ABI">;
+def err_unsupported_opt_for_execute_only_target
+    : Error<"unsupported option '%0' for the execute only target '%1'">;
----------------
MaggieYi wrote:
> MaskRay wrote:
> > We don't need this diagnostic as a common kind (we only use it in driver).
> > 
> > I think we can reuse `err_drv_argument_not_allowed_with` . Though for PS5 you will get `... allowed with '-mexecute-only'` even if the user doesn't specify `-mexecute-only`, but I hope it is fine.
> Since `err_drv_argument_not_allowed_with` is an ARM-only option, We cannot reuse it.
`err_drv_argument_not_allowed_with` is a generic diagnostic. My point is that we don't need an extra err_unsupported_opt_for_execute_only_target.


================
Comment at: clang/lib/Basic/Sanitizers.cpp:126
+  // execute-only output (no data access to code sections).
+  if (const llvm::opt::Arg *A =
+          Args.getLastArg(clang::driver::options::OPT_mexecute_only,
----------------
The idiom is `hasFlag(clang::driver::options::OPT_mexecute_only, clang::driver::options::OPT_mno_execute_only, false)`


================
Comment at: clang/lib/Basic/Sanitizers.cpp:130
+    if (A->getOption().matches(clang::driver::options::OPT_mexecute_only) &&
+        llvm::ARM::supportedExecuteOnly(Triple)) {
+      return true;
----------------
I don't think we need an extra `llvm::ARM::supportedExecuteOnly` check. We just return true when `-mexecute-only` is in effect.


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:478
       }
+      // `-fsanitize=function` is silently discarded on an execute-only target
+      // if implicitly enabled through group expansion.
----------------
`-fsanitize=function => NotAllowedWithExecuteOnly

since we now handle kcfi as well.


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:481
+      if (isExecuteOnlyTarget(Triple, Args)) {
+        Add &= ~NotAllowedWithExecuteOnly;
+      }
----------------
omit braces


================
Comment at: clang/test/Driver/fsanitize.c:981
+// RUN: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=function %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-FUNCTION
+// RUN: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=undefined -fsanitize=function %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-FUNCTION
+// RUN: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=kcfi %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-KCFI
----------------
Drop `not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=undefined -fsanitize=function`. 
Testing just `-fsanitize=function` for the negative test is sufficient.


================
Comment at: clang/test/Driver/fsanitize.c:983
+// RUN: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=kcfi %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-KCFI
+// RUN: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=function -fsanitize=kcfi %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-KCFI --check-prefix=CHECK-UBSAN-FUNCTION
+// RUN: %clang --target=armv6t2-eabi -mexecute-only -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-UNDEFINED
----------------
Drop `-fsanitize=function -fsanitize=kcfi` line. They already lead to an error.


================
Comment at: clang/test/Driver/fsanitize.c:984
+// RUN: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=function -fsanitize=kcfi %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-KCFI --check-prefix=CHECK-UBSAN-FUNCTION
+// RUN: %clang --target=armv6t2-eabi -mexecute-only -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-UNDEFINED
+
----------------
`--target=armv6t2-eabi -mexecute-only -fsanitize=undefined` needs a custom check prefix that checks `function` is not enabled.


================
Comment at: llvm/lib/TargetParser/ARMTargetParser.cpp:602
+
+bool ARM::supportedExecuteOnly(const Triple &TT) {
+  if (parseArchVersion(TT.getArchName()) < 7 &&
----------------
We don't need to extract the function.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158614/new/

https://reviews.llvm.org/D158614