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

[Ying Yi via Phabricator via llvm-commits]

MaggieYi added a comment.

Thanks  @simon_tatham and @MaskRay for the quick code review.

When I work on this issue, I want to add an error for both clang and clang -cc1. 
`-mexecute-only` is an ARM-only compiler option. The clang Driver will convert `-mexecute-only` to `-target-feature +execute-only` in the clang cc1 command.
To pass the `execute-only` option, the clang command is: `clang -target=armv6t2-eabi -mexecute-only …`. The Arm clang cc1 command:
`clang -cc1 -triple armv6t2-unknown-unknown-eabi -target-feature +execute-only…`.

If I move the change to the code in Driver/ToolChains/Arch/ARM.cpp, the error will only deal with the `-mexecute-only` option, but not handle the `-target-feature +execute-only` in the `clang -cc1` command.

As @MaskRay commented that we don't perform rigid error checking for cc1. In this case, we could handle the error in the Driver/ToolChains/Arch/ARM.cpp. However, the target-specific predicate function (named isExecuteOnlyTarget) allows any other targets that support execute-only could get the effect by modifying just the `isExecuteOnlyTarget` function. Therefore, I have modified the `isExecuteOnlyTarget` function to only deal with the `-mexecute-only` option. I have also added a function (named ARM::supportedExecuteOnly) to avoid the duplicated code.



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


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:406
+        if (SanitizerMask KindsToDiagnose =
+                Add & NotAllowedWithExecuteOnly & ~DiagnosedKinds) {
+          if (DiagnoseErrors)
----------------
MaskRay wrote:
> 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.
`NotAllowedWithExecuteOnly` is modified to include the `SanitizerKind::KCFI`, therefore I keep it in the code.


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

https://reviews.llvm.org/D158614