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