[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'">;
----------------
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.


================
Comment at: clang/lib/Basic/Sanitizers.cpp:134-143
+  if ((A &&
+       A->getOption().matches(clang::driver::options::OPT_mexecute_only)) ||
+      (std::find(Features.begin(), Features.end(), "+execute-only") !=
+       Features.end())) {
+    // The execute-only output is supported only on ARMv6T2 and ARMv7 and above.
+    if (llvm::ARM::parseArchVersion(Triple.getArchName()) > 7 ||
+        llvm::ARM::parseArch(Triple.getArchName()) ==
----------------
simon_tatham wrote:
> Why do we need to check //both// of `-mexecute-only` and the `+execute-only` target feature? It looks to me as if the code in `Driver/ToolChains/Arch/ARM.cpp` that handles `-mexecute-only` ends up setting the same target feature anyway. And it checks the supported architecture versions first.
> 
> Would it not be better to //just// check the target feature here, and avoid having a duplicated copy of the rest of this logic which needs to be kept in sync with the ARM driver?
> 
> Does something go wrong if you do it that way?
I think we only need to check the `+execute-only` target feature and remove driver option `-mexecute-only` check.

```
if (Triple.isPS5())
  return true;
if (!Triple.isARM() && !Triple.isThumb())
  return false;
return features contains "+execute-only" ;
```


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:406
+        if (SanitizerMask KindsToDiagnose =
+                Add & NotAllowedWithExecuteOnly & ~DiagnosedKinds) {
+          if (DiagnoseErrors)
----------------
I think it's clear not not to add the variable `NotAllowedWithExecuteOnly`.

Currently, I need to check the definition of `NotAllowedWithExecuteOnly` to understand that this comment does what it says. For now, just encoding `Function` here is clearer.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4405
+  // value of '-fsanitize=' must be `function` if function sanitizer is enabled.
+  if (isExecuteOnlyTarget(T, Args) &&
+      LangOpts.Sanitize.has(SanitizerKind::Function)) {
----------------
Remove.

We don't perform rigid error checking for cc1. If the user bypass the driver check with `-Xclang -fsanitize=function`, we don't provide more diagnostics.


================
Comment at: clang/test/CodeGen/ubsan-function.c:2
 // RUN: %clang_cc1 -emit-llvm -triple x86_64 -std=c17 -fsanitize=function %s -o - | FileCheck %s
+// RUN: not %clang_cc1 -emit-llvm -triple x86_64-sie-ps5 -fsanitize=function %s -o 2>&1 | FileCheck %s --check-prefix=UBSAN-FUNCTION-ERR
+// RUN: not %clang_cc1 -emit-llvm -triple armv6t2-unknown-unknown-eabi -target-feature +execute-only -fsanitize=function %s -o 2>&1 | FileCheck %s --check-prefix=UBSAN-FUNCTION-ERR
----------------
remove new tests. we only need test/Driver test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158614