Skip to content

Conversation

@thurstond
Copy link
Contributor

parseSanitizeTrapArgs follows the general pattern of "compute the sanitizer mask based on the default plus opt-in (if supported) minus opt-out". This patch refactors the functionality into a generalized function, parseSanitizeArgs, which will be useful for future sanitizer flag parsing.

parseSanitizeTrapArgs follows the general pattern of "compute the sanitizer mask based on the default plus opt-in (if supported) minus opt-out". This patch refactors the functionality into a generalized function, parseSanitizeArgs, which will be useful for future sanitizer flag parsing.
@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 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 13, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Thurston Dang (thurstond)

Changes

parseSanitizeTrapArgs follows the general pattern of "compute the sanitizer mask based on the default plus opt-in (if supported) minus opt-out". This patch refactors the functionality into a generalized function, parseSanitizeArgs, which will be useful for future sanitizer flag parsing.


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

1 Files Affected:

  • (modified) clang/lib/Driver/SanitizerArgs.cpp (+28-18)
diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index e77857930996b2..355dea5fad80bf 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -247,39 +247,49 @@ static SanitizerMask setGroupBits(SanitizerMask Kinds) {
   return Kinds;
 }
 
-static SanitizerMask parseSanitizeTrapArgs(const Driver &D,
-                                           const llvm::opt::ArgList &Args,
-                                           bool DiagnoseErrors) {
-  SanitizerMask TrapRemove; // During the loop below, the accumulated set of
-                            // sanitizers disabled by the current sanitizer
-                            // argument or any argument after it.
-  SanitizerMask TrappingKinds;
-  SanitizerMask TrappingSupportedWithGroups = setGroupBits(TrappingSupported);
+// Computes the sanitizer mask based on the default plus opt-in (if supported)
+// minus opt-out.
+static SanitizerMask
+parseSanitizeArgs(const Driver &D, const llvm::opt::ArgList &Args,
+                  bool DiagnoseErrors, SanitizerMask Supported,
+                  SanitizerMask Default, int OptInID, int OptOutID) {
+  SanitizerMask Remove; // During the loop below, the accumulated set of
+                        // sanitizers disabled by the current sanitizer
+                        // argument or any argument after it.
+  SanitizerMask Kinds;
+  SanitizerMask SupportedWithGroups = setGroupBits(Supported);
 
   for (const llvm::opt::Arg *Arg : llvm::reverse(Args)) {
-    if (Arg->getOption().matches(options::OPT_fsanitize_trap_EQ)) {
+    if (Arg->getOption().matches(OptInID)) {
       Arg->claim();
       SanitizerMask Add = parseArgValues(D, Arg, true);
-      Add &= ~TrapRemove;
-      SanitizerMask InvalidValues = Add & ~TrappingSupportedWithGroups;
+      Add &= ~Remove;
+      SanitizerMask InvalidValues = Add & ~SupportedWithGroups;
       if (InvalidValues && DiagnoseErrors) {
         SanitizerSet S;
         S.Mask = InvalidValues;
         D.Diag(diag::err_drv_unsupported_option_argument)
             << Arg->getSpelling() << toString(S);
       }
-      TrappingKinds |= expandSanitizerGroups(Add) & ~TrapRemove;
-    } else if (Arg->getOption().matches(options::OPT_fno_sanitize_trap_EQ)) {
+      Kinds |= expandSanitizerGroups(Add) & ~Remove;
+    } else if (Arg->getOption().matches(OptOutID)) {
       Arg->claim();
-      TrapRemove |=
-          expandSanitizerGroups(parseArgValues(D, Arg, DiagnoseErrors));
+      Remove |= expandSanitizerGroups(parseArgValues(D, Arg, DiagnoseErrors));
     }
   }
 
-  // Apply default trapping behavior.
-  TrappingKinds |= TrappingDefault & ~TrapRemove;
+  // Apply default behavior.
+  Kinds |= Default & ~Remove;
+
+  return Kinds;
+}
 
-  return TrappingKinds;
+static SanitizerMask parseSanitizeTrapArgs(const Driver &D,
+                                           const llvm::opt::ArgList &Args,
+                                           bool DiagnoseErrors) {
+  return parseSanitizeArgs(D, Args, DiagnoseErrors, TrappingSupported,
+                           TrappingDefault, options::OPT_fsanitize_trap_EQ,
+                           options::OPT_fno_sanitize_trap_EQ);
 }
 
 bool SanitizerArgs::needsFuzzerInterceptors() const {

@vitalybuka
Copy link
Collaborator

Would be possible to switch DiagnosedAlwaysRecoverableKinds to this function?

@thurstond
Copy link
Contributor Author

Would be possible to switch DiagnosedAlwaysRecoverableKinds to this function?

That's a bit more complicated because it has Unrecoverable and AlwaysRecoverable error cases (vs. the current function only has one error case: ~Supported). I'll refactor RecoverableKinds in a follow-up patch.

@thurstond thurstond merged commit 7077896 into llvm:main Dec 13, 2024
9 of 11 checks passed
@thurstond
Copy link
Contributor Author

Would be possible to switch DiagnosedAlwaysRecoverableKinds to this function?

That's a bit more complicated because it has Unrecoverable and AlwaysRecoverable error cases (vs. the current function only has one error case: ~Supported). I'll refactor RecoverableKinds in a follow-up patch.

#119819

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.

3 participants