Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang] Improve error for -fsanitize=function/kcfi -mexecute-only incompatibility #118816

Conversation

Il-Capitano
Copy link
Contributor

@Il-Capitano Il-Capitano commented Dec 5, 2024

The current error message when using the -fsanitize=function -mexecute-only flags together points to the target triple as the reason that -fsanitize=function is not allowed to be used, even when the function sanitizer is otherwise supported on the target when not using -mexecute-only.

The error message is improved to give -mexecute-only as the reason for disallowing -fsanitize=function if it was passed to the driver.

Fixes #117974

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Dec 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 5, 2024

@llvm/pr-subscribers-clang

Author: Csanád Hajdú (Il-Capitano)

Changes

The current error message when using the -fsanitize=function -mexecute-only flags together points to the target triple as the reason that -fsanitize=function is not allowed to be used, even when the function sanitizer is otherwise supported on the target when not using -mexecute-only.

The error message is improved to give -mexecute-only as the reason for disallowing -fsanitize=function if it was passed to the driver.


Full diff: https://github.com/llvm/llvm-project/pull/118816.diff

2 Files Affected:

  • (modified) clang/lib/Driver/SanitizerArgs.cpp (+8-2)
  • (modified) clang/test/Driver/fsanitize.c (+2-2)
diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index 1abfe8fd92807e..6d87e25b49237e 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -418,8 +418,14 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
                 Add & NotAllowedWithExecuteOnly & ~DiagnosedKinds) {
           if (DiagnoseErrors) {
             std::string Desc = describeSanitizeArg(Arg, KindsToDiagnose);
-            D.Diag(diag::err_drv_argument_not_allowed_with)
-                << Desc << Triple.str();
+                llvm::opt::Arg *A = Args.getLastArgNoClaim(
+                options::OPT_mexecute_only, options::OPT_mno_execute_only);
+            if (A && A->getOption().matches(options::OPT_mexecute_only))
+              D.Diag(diag::err_drv_argument_not_allowed_with)
+                  << Desc << A->getAsString(Args);
+            else
+              D.Diag(diag::err_drv_argument_not_allowed_with)
+                  << Desc << Triple.str();
           }
           DiagnosedKinds |= KindsToDiagnose;
         }
diff --git a/clang/test/Driver/fsanitize.c b/clang/test/Driver/fsanitize.c
index 15f190165a7d73..a99bfb3e82ac95 100644
--- a/clang/test/Driver/fsanitize.c
+++ b/clang/test/Driver/fsanitize.c
@@ -1000,8 +1000,8 @@
 // RUN: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=kcfi %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-KCFI
 // RUN: %clang --target=armv6t2-eabi -mexecute-only -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-UNDEFINED-VPTR
 
-// CHECK-UBSAN-KCFI-DAG: error: invalid argument '-fsanitize=kcfi' not allowed with {{('x86_64-sie-ps5'|'armv6t2-unknown-unknown-eabi')}}
-// CHECK-UBSAN-FUNCTION-DAG: error: invalid argument '-fsanitize=function' not allowed with {{('x86_64-sie-ps5'|'armv6t2-unknown-unknown-eabi')}}
+// CHECK-UBSAN-KCFI-DAG: error: invalid argument '-fsanitize=kcfi' not allowed with {{('x86_64-sie-ps5'|'-mexecute-only')}}
+// CHECK-UBSAN-FUNCTION-DAG: error: invalid argument '-fsanitize=function' not allowed with {{('x86_64-sie-ps5'|'-mexecute-only')}}
 // CHECK-UBSAN-UNDEFINED-VPTR: "-fsanitize={{((alignment|array-bounds|bool|builtin|enum|float-cast-overflow|integer-divide-by-zero|nonnull-attribute|null|pointer-overflow|return|returns-nonnull-attribute|shift-base|shift-exponent|signed-integer-overflow|unreachable|vla-bound|vptr),?){18}"}}
 
 // * Test BareMetal toolchain sanitizer support *

@llvmbot
Copy link
Member

llvmbot commented Dec 5, 2024

@llvm/pr-subscribers-clang-driver

Author: Csanád Hajdú (Il-Capitano)

Changes

The current error message when using the -fsanitize=function -mexecute-only flags together points to the target triple as the reason that -fsanitize=function is not allowed to be used, even when the function sanitizer is otherwise supported on the target when not using -mexecute-only.

The error message is improved to give -mexecute-only as the reason for disallowing -fsanitize=function if it was passed to the driver.


Full diff: https://github.com/llvm/llvm-project/pull/118816.diff

2 Files Affected:

  • (modified) clang/lib/Driver/SanitizerArgs.cpp (+8-2)
  • (modified) clang/test/Driver/fsanitize.c (+2-2)
diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index 1abfe8fd92807e..6d87e25b49237e 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -418,8 +418,14 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
                 Add & NotAllowedWithExecuteOnly & ~DiagnosedKinds) {
           if (DiagnoseErrors) {
             std::string Desc = describeSanitizeArg(Arg, KindsToDiagnose);
-            D.Diag(diag::err_drv_argument_not_allowed_with)
-                << Desc << Triple.str();
+                llvm::opt::Arg *A = Args.getLastArgNoClaim(
+                options::OPT_mexecute_only, options::OPT_mno_execute_only);
+            if (A && A->getOption().matches(options::OPT_mexecute_only))
+              D.Diag(diag::err_drv_argument_not_allowed_with)
+                  << Desc << A->getAsString(Args);
+            else
+              D.Diag(diag::err_drv_argument_not_allowed_with)
+                  << Desc << Triple.str();
           }
           DiagnosedKinds |= KindsToDiagnose;
         }
diff --git a/clang/test/Driver/fsanitize.c b/clang/test/Driver/fsanitize.c
index 15f190165a7d73..a99bfb3e82ac95 100644
--- a/clang/test/Driver/fsanitize.c
+++ b/clang/test/Driver/fsanitize.c
@@ -1000,8 +1000,8 @@
 // RUN: not %clang --target=armv6t2-eabi -mexecute-only -fsanitize=kcfi %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-KCFI
 // RUN: %clang --target=armv6t2-eabi -mexecute-only -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-UNDEFINED-VPTR
 
-// CHECK-UBSAN-KCFI-DAG: error: invalid argument '-fsanitize=kcfi' not allowed with {{('x86_64-sie-ps5'|'armv6t2-unknown-unknown-eabi')}}
-// CHECK-UBSAN-FUNCTION-DAG: error: invalid argument '-fsanitize=function' not allowed with {{('x86_64-sie-ps5'|'armv6t2-unknown-unknown-eabi')}}
+// CHECK-UBSAN-KCFI-DAG: error: invalid argument '-fsanitize=kcfi' not allowed with {{('x86_64-sie-ps5'|'-mexecute-only')}}
+// CHECK-UBSAN-FUNCTION-DAG: error: invalid argument '-fsanitize=function' not allowed with {{('x86_64-sie-ps5'|'-mexecute-only')}}
 // CHECK-UBSAN-UNDEFINED-VPTR: "-fsanitize={{((alignment|array-bounds|bool|builtin|enum|float-cast-overflow|integer-divide-by-zero|nonnull-attribute|null|pointer-overflow|return|returns-nonnull-attribute|shift-base|shift-exponent|signed-integer-overflow|unreachable|vla-bound|vptr),?){18}"}}
 
 // * Test BareMetal toolchain sanitizer support *

…ncompatibility

The current error message when using the `-fsanitize=function -mexecute-only`
flags together points to the target triple as the reason that
`-fsanitize=function` is not allowed to be used, even when the function
sanitizer is otherwise supported on the target when not using
`-mexecute-only`.

The error message is improved to give `-mexecute-only` as the reason for
disallowing `-fsanitize=function` if it was passed to the driver.
Copy link

github-actions bot commented Dec 5, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@Il-Capitano Il-Capitano force-pushed the clang-fsanitize-function-mexecute-only-diagnostic branch from 691a26b to 94c933e Compare December 5, 2024 14:49
@Il-Capitano
Copy link
Contributor Author

@MaskRay Can I ask you to review/add appropriate reviewers for this patch?

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, err_drv_unsupported_opt_for_target will be better for the incompatible triple error

@Il-Capitano
Copy link
Contributor Author

Thanks for reviewing!

I updated the code to use err_drv_unsupported_opt_for_target, and added test cases for the -mpure-code alias of -mexecute-only also.

@MaskRay MaskRay merged commit 1946d32 into llvm:main Dec 11, 2024
8 checks passed
@Il-Capitano Il-Capitano deleted the clang-fsanitize-function-mexecute-only-diagnostic branch December 12, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clang] Confusing diagnostic when using -fsanitize=function -mexecute-only flags together on ARM targets
3 participants