Skip to content

[WIP][DO NOT MERGE][Clang][Driver] Emit warning when -fsanitize-trap=<...> is passed without associated -fsanitize=<...> - #147997

Open
anthonyhatran wants to merge 1 commit into
llvm:mainfrom
anthonyhatran:sanitize-trap-clean
Open

[WIP][DO NOT MERGE][Clang][Driver] Emit warning when -fsanitize-trap=<...> is passed without associated -fsanitize=<...>#147997
anthonyhatran wants to merge 1 commit into
llvm:mainfrom
anthonyhatran:sanitize-trap-clean

Conversation

@anthonyhatran

@anthonyhatran anthonyhatran commented Jul 10, 2025

Copy link
Copy Markdown
Contributor

This PR is a work-in-progress (WIP) and should not be merged.

This is a sketch PR that will accompany an RFC about changing Clang's behavior handling -fsanitize-trap= without the corresponding sanitizer being enabled.

After parsing the sanitizer arguments and detecting a mismatched set of bits (-fsanitize-trap=<...> passed without corresponding -fsanitize=<...>), it is passed off to the helper, diagnoseTrapOnly. There, we utilize an X Macro pattern to first clear any sanitizer groups in the first pass, then clear any remaining individual sanitizer handlers in the second pass. This is intended to prevent flooding the user with warnings; for example if -fsanitize-trap=undefined is passed without -fsanitize=undefined, we don’t want every individual sanitizer inside the undefined group to emit a warning.

Part of a GSoC 2025 Project

@github-actions

Copy link
Copy Markdown

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 10, 2025
@llvmbot

llvmbot commented Jul 10, 2025

Copy link
Copy Markdown
Member

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Anthony Tran (anthonyhatran)

Changes

Part of a GSoC 2025 Project


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

2 Files Affected:

  • (modified) clang/include/clang/Basic/DiagnosticDriverKinds.td (+5)
  • (modified) clang/include/clang/Basic/DiagnosticGroups.td (+3)
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 34b6c0d7a8acd..49a8bdf06bda4 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -874,4 +874,9 @@ def warn_drv_openacc_without_cir
     : Warning<"OpenACC directives will result in no runtime behavior; use "
               "-fclangir to enable runtime effect">,
       InGroup<SourceUsesOpenACC>;
+
+def warn_drv_sanitize_trap_mismatch : Warning<
+  "-fsanitize-trap=%0 has no effect because the matching sanitizer is not enabled; "
+  "did you mean to pass \"-fsanitize=%0\" as well?">,
+  InGroup<SanitizeTrapMismatch>;
 }
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index f54a830b0103e..a79562cd9c2e0 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1752,3 +1752,6 @@ def ExplicitSpecializationStorageClass : DiagGroup<"explicit-specialization-stor
 
 // A warning for options that enable a feature that is not yet complete
 def ExperimentalOption : DiagGroup<"experimental-option">;
+
+// Warnings for sanitizer arguments
+def SanitizeTrapMismatch : DiagGroup<"sanitize-trap-mismatch">;

@llvmbot llvmbot added the clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' label Jul 10, 2025
@anthonyhatran anthonyhatran changed the title [Clang][Driver] Emit warning when -fsanitize-trap=undefined is passed without -fsanitize=undefined [Clang][Driver] Emit warning when -fsanitize-trap=<...> is passed without associated -fsanitize=<...> Jul 10, 2025
@MaskRay

MaskRay commented Jul 11, 2025

Copy link
Copy Markdown
Member

The current -fsanitize-trap behavior is intentional. See
https://discourse.llvm.org/t/clang-gsoc-2025-usability-improvements-for-trapping-undefined-behavior-sanitizer/84568/11

I believe Google actually depended on -fsanitize-trap=undefined not leading to warnings/errors when -fsanitize=undefined is not specified.

@MaskRay MaskRay closed this Jul 11, 2025
@Michael137

Copy link
Copy Markdown
Member

@MaskRay i think the intention of this PR was to write up an accompanying RFC. I don't think the linked Discourse thread had consensus on whether this was something we want to change or not

(summoning @delcypher)

Re-opening for now.

@Michael137 Michael137 reopened this Jul 11, 2025
@delcypher delcypher changed the title [Clang][Driver] Emit warning when -fsanitize-trap=<...> is passed without associated -fsanitize=<...> [WIP][DO NOT MERGE][Clang][Driver] Emit warning when -fsanitize-trap=<...> is passed without associated -fsanitize=<...> Jul 11, 2025
@delcypher

Copy link
Copy Markdown
Contributor

@MaskRay

The current -fsanitize-trap behavior is intentional. See
https://discourse.llvm.org/t/clang-gsoc-2025-usability-improvements-for-trapping-undefined-behavior-sanitizer/84568/11

Yes we aware the current behavior is intentional. That however doesn't mean it's the right behavior. Our intention is to post a corresponding RFC but as part of the RFC we also wanted a sketch patch of what we are proposing so we can be concrete about our proposed alternative. We (myself & @Michael137) are working with a @anthonyhatran a Google Summer of Code mentee to get the sketch patch in good shape and then we will work on an RFC and then post it.

Although I understand you have good intentions behind proactively closing the PR please do not close our mentee's PR again. If it's ultimately rejected, we'll do it.

@delcypher delcypher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good for the sketch patch. I have a few minor questions and nits that I've left. Please add a few driver test cases to illustrate how you expect this to work (including a test case that suppresses the warning).

Comment thread clang/lib/Driver/SanitizerArgs.cpp Outdated
Comment thread clang/lib/Driver/SanitizerArgs.cpp Outdated
Comment thread clang/include/clang/Basic/DiagnosticDriverKinds.td Outdated
Comment thread clang/lib/Driver/SanitizerArgs.cpp Outdated
@MaskRay

MaskRay commented Jul 12, 2025

Copy link
Copy Markdown
Member

Thank you for your efforts to enhance sanitizers. However, the current implementation aligns with the preferences of many, including GCC contributors who introduced -fsanitize-trap in 2022. Speaking in my capacity as a clang driver maintainer, sanitizer contributor, and GCC committer, gaining consensus to modify -fsanitize-trap to issue warnings or errors seems unlikely, so closing this issue appears to be the appropriate action.

The proposed change could serve an educational purpose by illustrating modifications to the driver and sanitizer components. However, pursuing its development within llvm/llvm-project risks causing confusion among contributors.

@github-actions

github-actions Bot commented Aug 5, 2025

Copy link
Copy Markdown

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff HEAD~1 HEAD --extensions c,cpp -- clang/test/Driver/fsanitize-trap-mismatch.c clang/lib/Driver/SanitizerArgs.cpp
View the diff from clang-format here.
diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index 5d0921696..bf3eef7ab 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -754,13 +754,13 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
       options::OPT_fno_sanitize_recover_EQ);
   RecoverableKinds &= Kinds;
 
-  // FIXME: `DiagnoseErrors` name seems a little wrong as we emit warnings here, 
+  // FIXME: `DiagnoseErrors` name seems a little wrong as we emit warnings here,
   // not errors but that seems to be done elsewhere in this method.
 
   // Parse any -fsanitize-trap=<...> flags the user provided, then
   // diagnose any which do not have a matching -fsanitize=<...>
   if (DiagnoseErrors) {
-    // parseSanitizeTrapArgs was not used because it sets a TrappingDefault 
+    // parseSanitizeTrapArgs was not used because it sets a TrappingDefault
     // which causes the emission of warnings beyond what the user entered
     SanitizerMask ExplicitTrap = parseSanitizeArgs(
         D, Args, false, {}, {}, {}, options::OPT_fsanitize_trap_EQ,

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:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants