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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
