https://github.com/thurstond updated https://github.com/llvm/llvm-project/pull/119819
>From 9afe71655814ead9bd29acf5ebd515253777081d Mon Sep 17 00:00:00 2001 From: Thurston Dang <thurs...@google.com> Date: Fri, 13 Dec 2024 05:14:56 +0000 Subject: [PATCH 1/7] [sanitizer] Refactor -f(no-)?sanitize-recover parsing This moves the functionality into a generic parseSanitizerArgs function, and then uses it for parsing both -f(no-)?sanitize-recover and -f(no-)?sanitize-trap. Note: for backwards compatibility, we compute the sanitizer mask as: Default + AlwaysIn + Arguments - AlwaysOut instead of: Default + Arguments + AlwaysIn - AlwaysOut --- clang/lib/Driver/SanitizerArgs.cpp | 120 ++++++++++++++--------------- 1 file changed, 56 insertions(+), 64 deletions(-) diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp index 355dea5fad80bf..3e881fdf03ed7d 100644 --- a/clang/lib/Driver/SanitizerArgs.cpp +++ b/clang/lib/Driver/SanitizerArgs.cpp @@ -247,48 +247,72 @@ static SanitizerMask setGroupBits(SanitizerMask Kinds) { return Kinds; } -// Computes the sanitizer mask based on the default plus opt-in (if supported) -// minus opt-out. +// Computes the sanitizer mask as: +// Default + AlwaysIn + Arguments - AlwaysOut +// with arguments parsed from left to right. +// +// Error messages are optionally printed if the AlwaysIn or AlwaysOut +// invariants are violated. 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)) { + bool DiagnoseErrors, SanitizerMask Default, + SanitizerMask AlwaysIn, SanitizerMask AlwaysOut, int OptInID, + int OptOutID) { + + SanitizerMask Output = Default | AlwaysIn; + // Keep track of which violations we have already reported, to avoid + // duplicate error messages. + SanitizerMask DiagnosedAlwaysInViolations; + SanitizerMask DiagnosedAlwaysOutViolations; + for (const auto *Arg : Args) { if (Arg->getOption().matches(OptInID)) { - Arg->claim(); - SanitizerMask Add = parseArgValues(D, Arg, true); - 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); + SanitizerMask Add = parseArgValues(D, Arg, DiagnoseErrors); + // Report error if user explicitly tries to opt-in to an always-out + // sanitizer. + if (SanitizerMask KindsToDiagnose = + Add & AlwaysOut & ~DiagnosedAlwaysOutViolations) { + if (DiagnoseErrors) { + SanitizerSet SetToDiagnose; + SetToDiagnose.Mask |= KindsToDiagnose; + D.Diag(diag::err_drv_unsupported_option_argument) + << Arg->getSpelling() << toString(SetToDiagnose); + DiagnosedAlwaysOutViolations |= KindsToDiagnose; + } } - Kinds |= expandSanitizerGroups(Add) & ~Remove; + Output |= expandSanitizerGroups(Add); + Arg->claim(); } else if (Arg->getOption().matches(OptOutID)) { + SanitizerMask Remove = parseArgValues(D, Arg, DiagnoseErrors); + // Report error if user explicitly tries to opt-out of an always-in + // sanitizer. + if (SanitizerMask KindsToDiagnose = + Remove & AlwaysIn & ~DiagnosedAlwaysInViolations) { + if (DiagnoseErrors) { + SanitizerSet SetToDiagnose; + SetToDiagnose.Mask |= KindsToDiagnose; + D.Diag(diag::err_drv_unsupported_option_argument) + << Arg->getSpelling() << toString(SetToDiagnose); + DiagnosedAlwaysInViolations |= KindsToDiagnose; + } + } + Output &= ~expandSanitizerGroups(Remove); Arg->claim(); - Remove |= expandSanitizerGroups(parseArgValues(D, Arg, DiagnoseErrors)); } } - // Apply default behavior. - Kinds |= Default & ~Remove; + Output &= ~AlwaysOut; - return Kinds; + return Output; } 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, + SanitizerMask AlwaysTrap; // Empty + SanitizerMask NeverTrap = ~(setGroupBits(TrappingSupported)); + + return parseSanitizeArgs(D, Args, DiagnoseErrors, TrappingDefault, AlwaysTrap, + NeverTrap, options::OPT_fsanitize_trap_EQ, options::OPT_fno_sanitize_trap_EQ); } @@ -652,44 +676,12 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, // default in ASan? // Parse -f(no-)?sanitize-recover flags. - SanitizerMask RecoverableKinds = RecoverableByDefault | AlwaysRecoverable; - SanitizerMask DiagnosedUnrecoverableKinds; - SanitizerMask DiagnosedAlwaysRecoverableKinds; - for (const auto *Arg : Args) { - if (Arg->getOption().matches(options::OPT_fsanitize_recover_EQ)) { - SanitizerMask Add = parseArgValues(D, Arg, DiagnoseErrors); - // Report error if user explicitly tries to recover from unrecoverable - // sanitizer. - if (SanitizerMask KindsToDiagnose = - Add & Unrecoverable & ~DiagnosedUnrecoverableKinds) { - SanitizerSet SetToDiagnose; - SetToDiagnose.Mask |= KindsToDiagnose; - if (DiagnoseErrors) - D.Diag(diag::err_drv_unsupported_option_argument) - << Arg->getSpelling() << toString(SetToDiagnose); - DiagnosedUnrecoverableKinds |= KindsToDiagnose; - } - RecoverableKinds |= expandSanitizerGroups(Add); - Arg->claim(); - } else if (Arg->getOption().matches(options::OPT_fno_sanitize_recover_EQ)) { - SanitizerMask Remove = parseArgValues(D, Arg, DiagnoseErrors); - // Report error if user explicitly tries to disable recovery from - // always recoverable sanitizer. - if (SanitizerMask KindsToDiagnose = - Remove & AlwaysRecoverable & ~DiagnosedAlwaysRecoverableKinds) { - SanitizerSet SetToDiagnose; - SetToDiagnose.Mask |= KindsToDiagnose; - if (DiagnoseErrors) - D.Diag(diag::err_drv_unsupported_option_argument) - << Arg->getSpelling() << toString(SetToDiagnose); - DiagnosedAlwaysRecoverableKinds |= KindsToDiagnose; - } - RecoverableKinds &= ~expandSanitizerGroups(Remove); - Arg->claim(); - } - } + SanitizerMask RecoverableKinds = parseSanitizeArgs( + D, Args, DiagnoseErrors, RecoverableByDefault, AlwaysRecoverable, + Unrecoverable, options::OPT_fsanitize_recover_EQ, + options::OPT_fno_sanitize_recover_EQ); + RecoverableKinds &= Kinds; - RecoverableKinds &= ~Unrecoverable; TrappingKinds &= Kinds; RecoverableKinds &= ~TrappingKinds; >From c362e66e1045bd1054363655c55aa53482dc5aee Mon Sep 17 00:00:00 2001 From: Thurston Dang <thurs...@google.com> Date: Tue, 17 Dec 2024 18:41:33 +0000 Subject: [PATCH 2/7] Address feedback: add assert and enforce AlwaysIn --- clang/lib/Driver/SanitizerArgs.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp index 3e881fdf03ed7d..fd7e49b396a551 100644 --- a/clang/lib/Driver/SanitizerArgs.cpp +++ b/clang/lib/Driver/SanitizerArgs.cpp @@ -258,8 +258,10 @@ parseSanitizeArgs(const Driver &D, const llvm::opt::ArgList &Args, bool DiagnoseErrors, SanitizerMask Default, SanitizerMask AlwaysIn, SanitizerMask AlwaysOut, int OptInID, int OptOutID) { + assert(!(AlwaysIn & AlwaysOut) && + "parseSanitizeArgs called with contradictory in/out requirements"); - SanitizerMask Output = Default | AlwaysIn; + SanitizerMask Output = Default; // Keep track of which violations we have already reported, to avoid // duplicate error messages. SanitizerMask DiagnosedAlwaysInViolations; @@ -300,6 +302,7 @@ parseSanitizeArgs(const Driver &D, const llvm::opt::ArgList &Args, } } + Output |= AlwaysIn; Output &= ~AlwaysOut; return Output; >From 6a4a8ce693b3b9d019eb5adaeff4b42e7eefb905 Mon Sep 17 00:00:00 2001 From: Thurston Dang <thurs...@google.com> Date: Tue, 17 Dec 2024 22:55:22 +0000 Subject: [PATCH 3/7] Add 'AlwaysOutAdvisoryOnly' for backwards compatibility. Fixes Clang.Driver/fsanitize.c error reported by https://buildkite.com/llvm-project/github-pull-requests/builds/129870 --- clang/lib/Driver/SanitizerArgs.cpp | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp index fd7e49b396a551..f317cad00b9faa 100644 --- a/clang/lib/Driver/SanitizerArgs.cpp +++ b/clang/lib/Driver/SanitizerArgs.cpp @@ -248,16 +248,18 @@ static SanitizerMask setGroupBits(SanitizerMask Kinds) { } // Computes the sanitizer mask as: -// Default + AlwaysIn + Arguments - AlwaysOut +// Default + Arguments (in or out) + AlwaysIn - AlwaysOut // with arguments parsed from left to right. // +// AlwaysOut is not enforced if AlwaysOutAdvisoryOnly is enabled. +// // Error messages are optionally printed if the AlwaysIn or AlwaysOut // invariants are violated. static SanitizerMask parseSanitizeArgs(const Driver &D, const llvm::opt::ArgList &Args, bool DiagnoseErrors, SanitizerMask Default, SanitizerMask AlwaysIn, SanitizerMask AlwaysOut, int OptInID, - int OptOutID) { + int OptOutID, bool AlwaysOutAdvisoryOnly) { assert(!(AlwaysIn & AlwaysOut) && "parseSanitizeArgs called with contradictory in/out requirements"); @@ -303,7 +305,8 @@ parseSanitizeArgs(const Driver &D, const llvm::opt::ArgList &Args, } Output |= AlwaysIn; - Output &= ~AlwaysOut; + if (!AlwaysOutAdvisoryOnly) + Output &= ~AlwaysOut; return Output; } @@ -314,9 +317,13 @@ static SanitizerMask parseSanitizeTrapArgs(const Driver &D, SanitizerMask AlwaysTrap; // Empty SanitizerMask NeverTrap = ~(setGroupBits(TrappingSupported)); + // AlwaysOutAdvisoryOnly = true is needed to maintain the behavior of + // '-fsanitize=undefined -fsanitize-trap=undefined' + // (clang/test/Driver/fsanitize.c ), which is that vptr is not enabled at all + // (not even in recover mode) in order to avoid the need for a ubsan runtime. return parseSanitizeArgs(D, Args, DiagnoseErrors, TrappingDefault, AlwaysTrap, NeverTrap, options::OPT_fsanitize_trap_EQ, - options::OPT_fno_sanitize_trap_EQ); + options::OPT_fno_sanitize_trap_EQ, true); } bool SanitizerArgs::needsFuzzerInterceptors() const { @@ -682,7 +689,7 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, SanitizerMask RecoverableKinds = parseSanitizeArgs( D, Args, DiagnoseErrors, RecoverableByDefault, AlwaysRecoverable, Unrecoverable, options::OPT_fsanitize_recover_EQ, - options::OPT_fno_sanitize_recover_EQ); + options::OPT_fno_sanitize_recover_EQ, false); RecoverableKinds &= Kinds; >From c48088726f485a51280e738507cfe9a9d06cb65e Mon Sep 17 00:00:00 2001 From: Thurston Dang <thurs...@google.com> Date: Tue, 17 Dec 2024 22:58:24 +0000 Subject: [PATCH 4/7] Formatting --- clang/lib/Driver/SanitizerArgs.cpp | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp index f317cad00b9faa..87e2c2f8923351 100644 --- a/clang/lib/Driver/SanitizerArgs.cpp +++ b/clang/lib/Driver/SanitizerArgs.cpp @@ -306,7 +306,7 @@ parseSanitizeArgs(const Driver &D, const llvm::opt::ArgList &Args, Output |= AlwaysIn; if (!AlwaysOutAdvisoryOnly) - Output &= ~AlwaysOut; + Output &= ~AlwaysOut; return Output; } @@ -321,9 +321,13 @@ static SanitizerMask parseSanitizeTrapArgs(const Driver &D, // '-fsanitize=undefined -fsanitize-trap=undefined' // (clang/test/Driver/fsanitize.c ), which is that vptr is not enabled at all // (not even in recover mode) in order to avoid the need for a ubsan runtime. - return parseSanitizeArgs(D, Args, DiagnoseErrors, TrappingDefault, AlwaysTrap, - NeverTrap, options::OPT_fsanitize_trap_EQ, - options::OPT_fno_sanitize_trap_EQ, true); + return parseSanitizeArgs(D, Args, DiagnoseErrors, + /* Default */ TrappingDefault, + /* AlwaysIn */ AlwaysTrap, + /* AlwaysOut */ NeverTrap, + /* OptInID */ options::OPT_fsanitize_trap_EQ, + /* OptOutID */ options::OPT_fno_sanitize_trap_EQ, + /* AlwaysOutAdvisoryOnly */ true); } bool SanitizerArgs::needsFuzzerInterceptors() const { @@ -687,9 +691,13 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, // Parse -f(no-)?sanitize-recover flags. SanitizerMask RecoverableKinds = parseSanitizeArgs( - D, Args, DiagnoseErrors, RecoverableByDefault, AlwaysRecoverable, - Unrecoverable, options::OPT_fsanitize_recover_EQ, - options::OPT_fno_sanitize_recover_EQ, false); + D, Args, DiagnoseErrors, + /* Default */ RecoverableByDefault, + /* AlwaysIn */ AlwaysRecoverable, + /* AlwaysOut */ Unrecoverable, + /* OptInID */ options::OPT_fsanitize_recover_EQ, + /* OptOutID */ options::OPT_fno_sanitize_recover_EQ, + /* AlwaysOutAdvisoryOnly */ false); RecoverableKinds &= Kinds; >From e24fbd34558bd257d54e7aed4a85c5ffefdd5be5 Mon Sep 17 00:00:00 2001 From: Thurston Dang <thurs...@google.com> Date: Tue, 17 Dec 2024 23:01:43 +0000 Subject: [PATCH 5/7] clang-format --- clang/lib/Driver/SanitizerArgs.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp index 87e2c2f8923351..39b1c4d97b302d 100644 --- a/clang/lib/Driver/SanitizerArgs.cpp +++ b/clang/lib/Driver/SanitizerArgs.cpp @@ -690,14 +690,14 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, // default in ASan? // Parse -f(no-)?sanitize-recover flags. - SanitizerMask RecoverableKinds = parseSanitizeArgs( - D, Args, DiagnoseErrors, - /* Default */ RecoverableByDefault, - /* AlwaysIn */ AlwaysRecoverable, - /* AlwaysOut */ Unrecoverable, - /* OptInID */ options::OPT_fsanitize_recover_EQ, - /* OptOutID */ options::OPT_fno_sanitize_recover_EQ, - /* AlwaysOutAdvisoryOnly */ false); + SanitizerMask RecoverableKinds = + parseSanitizeArgs(D, Args, DiagnoseErrors, + /* Default */ RecoverableByDefault, + /* AlwaysIn */ AlwaysRecoverable, + /* AlwaysOut */ Unrecoverable, + /* OptInID */ options::OPT_fsanitize_recover_EQ, + /* OptOutID */ options::OPT_fno_sanitize_recover_EQ, + /* AlwaysOutAdvisoryOnly */ false); RecoverableKinds &= Kinds; >From 51973b75099d98a48e74bd9e2474e569c989932b Mon Sep 17 00:00:00 2001 From: Thurston Dang <thurs...@google.com> Date: Wed, 18 Dec 2024 00:02:23 +0000 Subject: [PATCH 6/7] Simplify per Vitaly's feedback --- clang/lib/Driver/SanitizerArgs.cpp | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp index 39b1c4d97b302d..b4b0aa04f3e5ef 100644 --- a/clang/lib/Driver/SanitizerArgs.cpp +++ b/clang/lib/Driver/SanitizerArgs.cpp @@ -248,18 +248,16 @@ static SanitizerMask setGroupBits(SanitizerMask Kinds) { } // Computes the sanitizer mask as: -// Default + Arguments (in or out) + AlwaysIn - AlwaysOut +// Default + Arguments (in or out) // with arguments parsed from left to right. // -// AlwaysOut is not enforced if AlwaysOutAdvisoryOnly is enabled. -// -// Error messages are optionally printed if the AlwaysIn or AlwaysOut -// invariants are violated. +// Error messages are printed if the AlwaysIn or AlwaysOut invariants are +// violated, but the caller must enforce these invariants themselves. static SanitizerMask parseSanitizeArgs(const Driver &D, const llvm::opt::ArgList &Args, bool DiagnoseErrors, SanitizerMask Default, SanitizerMask AlwaysIn, SanitizerMask AlwaysOut, int OptInID, - int OptOutID, bool AlwaysOutAdvisoryOnly) { + int OptOutID) { assert(!(AlwaysIn & AlwaysOut) && "parseSanitizeArgs called with contradictory in/out requirements"); @@ -304,10 +302,6 @@ parseSanitizeArgs(const Driver &D, const llvm::opt::ArgList &Args, } } - Output |= AlwaysIn; - if (!AlwaysOutAdvisoryOnly) - Output &= ~AlwaysOut; - return Output; } @@ -317,7 +311,7 @@ static SanitizerMask parseSanitizeTrapArgs(const Driver &D, SanitizerMask AlwaysTrap; // Empty SanitizerMask NeverTrap = ~(setGroupBits(TrappingSupported)); - // AlwaysOutAdvisoryOnly = true is needed to maintain the behavior of + // N.B. We do *not* enforce NeverTrap. This maintains the behavior of // '-fsanitize=undefined -fsanitize-trap=undefined' // (clang/test/Driver/fsanitize.c ), which is that vptr is not enabled at all // (not even in recover mode) in order to avoid the need for a ubsan runtime. @@ -326,8 +320,7 @@ static SanitizerMask parseSanitizeTrapArgs(const Driver &D, /* AlwaysIn */ AlwaysTrap, /* AlwaysOut */ NeverTrap, /* OptInID */ options::OPT_fsanitize_trap_EQ, - /* OptOutID */ options::OPT_fno_sanitize_trap_EQ, - /* AlwaysOutAdvisoryOnly */ true); + /* OptOutID */ options::OPT_fno_sanitize_trap_EQ); } bool SanitizerArgs::needsFuzzerInterceptors() const { @@ -696,9 +689,9 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, /* AlwaysIn */ AlwaysRecoverable, /* AlwaysOut */ Unrecoverable, /* OptInID */ options::OPT_fsanitize_recover_EQ, - /* OptOutID */ options::OPT_fno_sanitize_recover_EQ, - /* AlwaysOutAdvisoryOnly */ false); - + /* OptOutID */ options::OPT_fno_sanitize_recover_EQ); + RecoverableKinds |= AlwaysRecoverable; + RecoverableKinds &= ~Unrecoverable; RecoverableKinds &= Kinds; TrappingKinds &= Kinds; >From 686c25712b0b96c43d018986b9a3b257d9129b5c Mon Sep 17 00:00:00 2001 From: Thurston Dang <thurs...@google.com> Date: Wed, 18 Dec 2024 00:03:38 +0000 Subject: [PATCH 7/7] Remove comments --- clang/lib/Driver/SanitizerArgs.cpp | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp index b4b0aa04f3e5ef..0457d75b88405c 100644 --- a/clang/lib/Driver/SanitizerArgs.cpp +++ b/clang/lib/Driver/SanitizerArgs.cpp @@ -315,12 +315,9 @@ static SanitizerMask parseSanitizeTrapArgs(const Driver &D, // '-fsanitize=undefined -fsanitize-trap=undefined' // (clang/test/Driver/fsanitize.c ), which is that vptr is not enabled at all // (not even in recover mode) in order to avoid the need for a ubsan runtime. - return parseSanitizeArgs(D, Args, DiagnoseErrors, - /* Default */ TrappingDefault, - /* AlwaysIn */ AlwaysTrap, - /* AlwaysOut */ NeverTrap, - /* OptInID */ options::OPT_fsanitize_trap_EQ, - /* OptOutID */ options::OPT_fno_sanitize_trap_EQ); + return parseSanitizeArgs(D, Args, DiagnoseErrors, TrappingDefault, AlwaysTrap, + NeverTrap, options::OPT_fsanitize_trap_EQ, + options::OPT_fno_sanitize_trap_EQ); } bool SanitizerArgs::needsFuzzerInterceptors() const { @@ -683,13 +680,10 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, // default in ASan? // Parse -f(no-)?sanitize-recover flags. - SanitizerMask RecoverableKinds = - parseSanitizeArgs(D, Args, DiagnoseErrors, - /* Default */ RecoverableByDefault, - /* AlwaysIn */ AlwaysRecoverable, - /* AlwaysOut */ Unrecoverable, - /* OptInID */ options::OPT_fsanitize_recover_EQ, - /* OptOutID */ options::OPT_fno_sanitize_recover_EQ); + SanitizerMask RecoverableKinds = parseSanitizeArgs( + D, Args, DiagnoseErrors, RecoverableByDefault, AlwaysRecoverable, + Unrecoverable, options::OPT_fsanitize_recover_EQ, + options::OPT_fno_sanitize_recover_EQ); RecoverableKinds |= AlwaysRecoverable; RecoverableKinds &= ~Unrecoverable; RecoverableKinds &= Kinds; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits