* [PATCH RFA] ubsan: do return check with -fsanitize=unreachable @ 2022-06-17 21:20 Jason Merrill 2022-06-20 11:05 ` Jakub Jelinek 0 siblings, 1 reply; 9+ messages in thread From: Jason Merrill @ 2022-06-17 21:20 UTC (permalink / raw) To: gcc-patches, jakub Related to PR104642, the current situation where we get less return checking with just -fsanitize=unreachable than no sanitize flags seems undesirable; I propose that we do return checking when -fsanitize=unreachable. Looks like clang just traps on missing return if not -fsanitize=return, but the approach in this patch seems more helpful to me if we're already sanitizing other should-be-unreachable code. I'm assuming that the difference in treatment of SANITIZE_UNREACHABLE and SANITIZE_RETURN with regard to loop optimization is deliberate. This assumes Jakub's -fsanitize-trap patch. gcc/ChangeLog: * doc/invoke.texi: Note that -fsanitize=unreachable implies -fsanitize=return. * opts.cc (finish_options): Make that so. gcc/cp/ChangeLog: * cp-gimplify.cc (cp_maybe_instrument_return): Remove return vs. unreachable handling. gcc/testsuite/ChangeLog: * g++.dg/ubsan/return-8c.C: New test. --- gcc/doc/invoke.texi | 2 ++ gcc/cp/cp-gimplify.cc | 12 ------------ gcc/opts.cc | 10 ++++++++++ gcc/testsuite/g++.dg/ubsan/return-8c.C | 15 +++++++++++++++ 4 files changed, 27 insertions(+), 12 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ubsan/return-8c.C diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 50f57877477..e572158a1ba 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -15946,6 +15946,8 @@ built with this option turned on will issue an error message when the end of a non-void function is reached without actually returning a value. This option works in C++ only. +This check is also enabled by -fsanitize=unreachable. + @item -fsanitize=signed-integer-overflow @opindex fsanitize=signed-integer-overflow This option enables signed integer overflow checking. We check that diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc index 6f84d157c98..5c2eb61842c 100644 --- a/gcc/cp/cp-gimplify.cc +++ b/gcc/cp/cp-gimplify.cc @@ -1806,18 +1806,6 @@ cp_maybe_instrument_return (tree fndecl) || !targetm.warn_func_return (fndecl)) return; - if (!sanitize_flags_p (SANITIZE_RETURN, fndecl) - /* Don't add __builtin_unreachable () if not optimizing, it will not - improve any optimizations in that case, just break UB code. - Don't add it if -fsanitize=unreachable -fno-sanitize=return either, - UBSan covers this with ubsan_instrument_return above where sufficient - information is provided, while the __builtin_unreachable () below - if return sanitization is disabled will just result in hard to - understand runtime error without location. */ - && (!optimize - || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl))) - return; - tree t = DECL_SAVED_TREE (fndecl); while (t) { diff --git a/gcc/opts.cc b/gcc/opts.cc index 062782ac700..a7b02b0f693 100644 --- a/gcc/opts.cc +++ b/gcc/opts.cc @@ -1254,6 +1254,16 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set, if (opts->x_flag_sanitize & ~(SANITIZE_LEAK | SANITIZE_UNREACHABLE)) opts->x_flag_aggressive_loop_optimizations = 0; + /* -fsanitize=unreachable implies -fsanitize=return, but without affecting + aggressive loop optimizations. */ + if ((opts->x_flag_sanitize & (SANITIZE_UNREACHABLE | SANITIZE_RETURN)) + == SANITIZE_UNREACHABLE) + { + opts->x_flag_sanitize |= SANITIZE_RETURN; + if (opts->x_flag_sanitize_trap & SANITIZE_UNREACHABLE) + opts->x_flag_sanitize_trap |= SANITIZE_RETURN; + } + /* Enable -fsanitize-address-use-after-scope if either address sanitizer is enabled. */ if (opts->x_flag_sanitize diff --git a/gcc/testsuite/g++.dg/ubsan/return-8c.C b/gcc/testsuite/g++.dg/ubsan/return-8c.C new file mode 100644 index 00000000000..a67f086d452 --- /dev/null +++ b/gcc/testsuite/g++.dg/ubsan/return-8c.C @@ -0,0 +1,15 @@ +// PR c++/104642 + +// -fsanitize=unreachable should imply -fsanitize=return. + +// { dg-do run } +// { dg-shouldfail { *-*-* } } +// { dg-additional-options "-O -fsanitize=unreachable" } + +bool b; + +int f() { + if (b) return 42; +} // { dg-warning "-Wreturn-type" } + +int main() { f(); } base-commit: 0f96ac43fa0a5fdbfce317b274233852d5b46d23 prerequisite-patch-id: fa35013a253eae78fe744794172aeed26fe6f166 -- 2.27.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] ubsan: do return check with -fsanitize=unreachable 2022-06-17 21:20 [PATCH RFA] ubsan: do return check with -fsanitize=unreachable Jason Merrill @ 2022-06-20 11:05 ` Jakub Jelinek 2022-06-20 20:16 ` Jason Merrill 0 siblings, 1 reply; 9+ messages in thread From: Jakub Jelinek @ 2022-06-20 11:05 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches On Fri, Jun 17, 2022 at 05:20:02PM -0400, Jason Merrill wrote: > Related to PR104642, the current situation where we get less return checking > with just -fsanitize=unreachable than no sanitize flags seems undesirable; I > propose that we do return checking when -fsanitize=unreachable. __builtin_unreachable itself (unless turned into trap or __ubsan_handle_builtin_unreachable) is not any kind of return checking, it is just an optimization. > Looks like clang just traps on missing return if not -fsanitize=return, but > the approach in this patch seems more helpful to me if we're already > sanitizing other should-be-unreachable code. > > I'm assuming that the difference in treatment of SANITIZE_UNREACHABLE and > SANITIZE_RETURN with regard to loop optimization is deliberate. return and unreachable are separate sanitizers and such silent one way implication can have quite unexpected consequences, especially with -fsanitize-trap=. Say with -fsanitize=unreachable -fsanitize-trap=unreachable, both current trunk and clang will link without -lubsan, because the only enabled UBSan sanitizers use __builtin_trap () which doesn't need library. With -fsanitize=unreachable silently meaning -fsanitize=unreachable,return the above would link in -lubsan, because while SANITIZE_UNREACHABLE uses __builtin_trap, SANITIZE_RETURN doesn't. Similarly, one has no_sanitize attribute, one could in certain function __attribute__((no_sanitize ("unreachable"))) and because on the command line using -fsanitize=unreachable assume other sanitizers aren't enabled, but the silent addition of return sanitizer would break that. > --- a/gcc/cp/cp-gimplify.cc > +++ b/gcc/cp/cp-gimplify.cc > @@ -1806,18 +1806,6 @@ cp_maybe_instrument_return (tree fndecl) > || !targetm.warn_func_return (fndecl)) > return; > > - if (!sanitize_flags_p (SANITIZE_RETURN, fndecl) > - /* Don't add __builtin_unreachable () if not optimizing, it will not > - improve any optimizations in that case, just break UB code. > - Don't add it if -fsanitize=unreachable -fno-sanitize=return either, > - UBSan covers this with ubsan_instrument_return above where sufficient > - information is provided, while the __builtin_unreachable () below > - if return sanitization is disabled will just result in hard to > - understand runtime error without location. */ > - && (!optimize > - || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl))) > - return; > - > tree t = DECL_SAVED_TREE (fndecl); > while (t) > { I think the above is correct, if -fsanitize=return, we want to fall through and use __ubsan_handle_missing_return (or __builtin_trap if -fsanitize-trap=return). Otherwise, for -O0, __builtin_unreachable most likely doesn't offer any important optimization benefits and just makes debugging bad code harder. Similarly for -fsanitize=unreachable, the __builtin_unreachable there would be an optimization which we shouldn't turn into __ubsan_handle_builtin_unreachable / __builtin_trap. Now, -funreachable-traps can of course change the condition a little bit, and so can implementation of builtin_decl_unreachable and stopping of folding of __builtin_unreachable to __builtin_trap if -fsanitize=unreachable -fsanitize-trap=unreachable. The -fsanitize=return case remains the same no matter what. Otherwise, if -funreachable-traps, we are emitting __builtin_trap rather than __builtin_unreachable, so it is perfectly fine to fall through regardless of !optimize or SANITIZE_UNREACHABLE being on, it isn't an optimization in that case, but checking. Otherwise, if !optimize, we should return, __builtin_unreachable in there wouldn't bring many advantages and just punish users of bad code. Otherwise, if builtin_decl_unreachable is implemented and we never fold __builtin_unreachable to __builtin_trap, for SANITIZE_UNREACHABLE enabled and (flag_sanitize_trap & SANITIZE_UNREACHABLE) != 0 we could emit __builtin_unreachable (but in that case directly, not through builtin_decl_unreachable). Otherwise, if SANITIZE_UNREACHABLE is on and (flag_sanitize_trap & SANITIZE_UNREACHABLE) == 0, I assume we'll still want to fold __builtin_unreachable to __ubsan_handle_builtin_unreachable during sanopt etc., we can live without the optimization and not instrument. Otherwise emit __builtin_unreachable (directly). Jakub ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] ubsan: do return check with -fsanitize=unreachable 2022-06-20 11:05 ` Jakub Jelinek @ 2022-06-20 20:16 ` Jason Merrill 2022-06-22 4:04 ` Jason Merrill 0 siblings, 1 reply; 9+ messages in thread From: Jason Merrill @ 2022-06-20 20:16 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 1812 bytes --] On 6/20/22 07:05, Jakub Jelinek wrote: > On Fri, Jun 17, 2022 at 05:20:02PM -0400, Jason Merrill wrote: >> Related to PR104642, the current situation where we get less return checking >> with just -fsanitize=unreachable than no sanitize flags seems undesirable; I >> propose that we do return checking when -fsanitize=unreachable. > > __builtin_unreachable itself (unless turned into trap or > __ubsan_handle_builtin_unreachable) is not any kind of return checking, it > is just an optimization. Yes, but I'm talking about "when -fsanitize=unreachable". >> Looks like clang just traps on missing return if not -fsanitize=return, but >> the approach in this patch seems more helpful to me if we're already >> sanitizing other should-be-unreachable code. >> >> I'm assuming that the difference in treatment of SANITIZE_UNREACHABLE and >> SANITIZE_RETURN with regard to loop optimization is deliberate. > > return and unreachable are separate sanitizers and such silent one way > implication can have quite unexpected consequences, especially with > -fsanitize-trap=. > Say with -fsanitize=unreachable -fsanitize-trap=unreachable, both current > trunk and clang will link without -lubsan, because the only enabled UBSan > sanitizers use __builtin_trap () which doesn't need library. > With -fsanitize=unreachable silently meaning -fsanitize=unreachable,return > the above would link in -lubsan, because while SANITIZE_UNREACHABLE uses > __builtin_trap, SANITIZE_RETURN doesn't. > Similarly, one has no_sanitize attribute, one could in certain function > __attribute__((no_sanitize ("unreachable"))) and because on the command > line using -fsanitize=unreachable assume other sanitizers aren't enabled, > but the silent addition of return sanitizer would break that. Ah, true. How about this approach instead? [-- Attachment #2: 0001-ubsan-do-return-check-with-fsanitize-unreachable.patch --] [-- Type: text/x-patch, Size: 3876 bytes --] From 439c645fa5197ccbfcb6fbbeda5772a8581c3a7e Mon Sep 17 00:00:00 2001 From: Jason Merrill <jason@redhat.com> Date: Wed, 15 Jun 2022 15:45:48 -0400 Subject: [PATCH] ubsan: do return check with -fsanitize=unreachable To: gcc-patches@gcc.gnu.org The current situation where we get less return checking with just -fsanitize=unreachable than no sanitize flags seems undesirable; I propose that we do return checking when -fsanitize=unreachable. Looks like clang just traps on missing return if not -fsanitize=return, but the approach in this patch seems more helpful to me if we're already sanitizing other believed-unreachable code. I took this approach rather than setting SANITIZE_RETURN in opts.cc so that attribute no_sanitize ("unreachable") will turn this behavior off as well. gcc/ChangeLog: * doc/invoke.texi: Note that -fsanitize=unreachable includes -fsanitize=return. gcc/c-family/ChangeLog: * c-ubsan.cc (ubsan_instrument_return): Also look for trap on unreachable. gcc/cp/ChangeLog: * cp-gimplify.cc (cp_maybe_instrument_return): Also instrument return if SANITIZE_UNREACHABLE. --- gcc/doc/invoke.texi | 2 ++ gcc/c-family/c-ubsan.cc | 5 ++++- gcc/cp/cp-gimplify.cc | 14 +------------- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 16a893ec1da..b6786df73b9 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -15950,6 +15950,8 @@ built with this option turned on will issue an error message when the end of a non-void function is reached without actually returning a value. This option works in C++ only. +This check is also enabled by -fsanitize=unreachable. + @item -fsanitize=signed-integer-overflow @opindex fsanitize=signed-integer-overflow This option enables signed integer overflow checking. We check that diff --git a/gcc/c-family/c-ubsan.cc b/gcc/c-family/c-ubsan.cc index 360ba82250c..ac6da558473 100644 --- a/gcc/c-family/c-ubsan.cc +++ b/gcc/c-family/c-ubsan.cc @@ -339,7 +339,10 @@ ubsan_instrument_vla (location_t loc, tree size) tree ubsan_instrument_return (location_t loc) { - if (flag_sanitize_trap & SANITIZE_RETURN) + /* C++ calls this for SANITIZE_RETURN|SANITIZE_UNREACHABLE. */ + unsigned mask = flag_sanitize & SANITIZE_RETURN; + if (!mask) mask = flag_sanitize & SANITIZE_UNREACHABLE; + if (flag_sanitize_trap & mask) /* pass_warn_function_return checks for BUILTINS_LOCATION. */ return build_call_expr_loc (BUILTINS_LOCATION, builtin_decl_explicit (BUILT_IN_TRAP), 0); diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc index 7b0465729a3..7214e4be175 100644 --- a/gcc/cp/cp-gimplify.cc +++ b/gcc/cp/cp-gimplify.cc @@ -1806,18 +1806,6 @@ cp_maybe_instrument_return (tree fndecl) || !targetm.warn_func_return (fndecl)) return; - if (!sanitize_flags_p (SANITIZE_RETURN, fndecl) - /* Don't add __builtin_unreachable () if not optimizing, it will not - improve any optimizations in that case, just break UB code. - Don't add it if -fsanitize=unreachable -fno-sanitize=return either, - UBSan covers this with ubsan_instrument_return above where sufficient - information is provided, while the __builtin_unreachable () below - if return sanitization is disabled will just result in hard to - understand runtime error without location. */ - && (!optimize - || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl))) - return; - tree t = DECL_SAVED_TREE (fndecl); while (t) { @@ -1861,7 +1849,7 @@ cp_maybe_instrument_return (tree fndecl) p = &BIND_EXPR_BODY (*p); location_t loc = DECL_SOURCE_LOCATION (fndecl); - if (sanitize_flags_p (SANITIZE_RETURN, fndecl)) + if (sanitize_flags_p (SANITIZE_RETURN|SANITIZE_UNREACHABLE, fndecl)) t = ubsan_instrument_return (loc); else t = build_builtin_unreachable (BUILTINS_LOCATION); -- 2.27.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] ubsan: do return check with -fsanitize=unreachable 2022-06-20 20:16 ` Jason Merrill @ 2022-06-22 4:04 ` Jason Merrill 2022-06-24 14:26 ` Jason Merrill 2022-06-27 15:44 ` Jakub Jelinek 0 siblings, 2 replies; 9+ messages in thread From: Jason Merrill @ 2022-06-22 4:04 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 2179 bytes --] On 6/20/22 16:16, Jason Merrill wrote: > On 6/20/22 07:05, Jakub Jelinek wrote: >> On Fri, Jun 17, 2022 at 05:20:02PM -0400, Jason Merrill wrote: >>> Related to PR104642, the current situation where we get less return >>> checking >>> with just -fsanitize=unreachable than no sanitize flags seems >>> undesirable; I >>> propose that we do return checking when -fsanitize=unreachable. >> >> __builtin_unreachable itself (unless turned into trap or >> __ubsan_handle_builtin_unreachable) is not any kind of return >> checking, it >> is just an optimization. > > Yes, but I'm talking about "when -fsanitize=unreachable". > >>> Looks like clang just traps on missing return if not >>> -fsanitize=return, but >>> the approach in this patch seems more helpful to me if we're already >>> sanitizing other should-be-unreachable code. >>> >>> I'm assuming that the difference in treatment of SANITIZE_UNREACHABLE >>> and >>> SANITIZE_RETURN with regard to loop optimization is deliberate. >> >> return and unreachable are separate sanitizers and such silent one way >> implication can have quite unexpected consequences, especially with >> -fsanitize-trap=. >> Say with -fsanitize=unreachable -fsanitize-trap=unreachable, both current >> trunk and clang will link without -lubsan, because the only enabled UBSan >> sanitizers use __builtin_trap () which doesn't need library. >> With -fsanitize=unreachable silently meaning >> -fsanitize=unreachable,return >> the above would link in -lubsan, because while SANITIZE_UNREACHABLE uses >> __builtin_trap, SANITIZE_RETURN doesn't. >> Similarly, one has no_sanitize attribute, one could in certain function >> __attribute__((no_sanitize ("unreachable"))) and because on the command >> line using -fsanitize=unreachable assume other sanitizers aren't enabled, >> but the silent addition of return sanitizer would break that. > > Ah, true. How about this approach instead? Or, this approach relies on the PR104642 patch, and just fixes the line number issue. This is less clear about the problem than using the return ubsan library function, but avoids using one entry point to implement the other sanitizer, if that's important. [-- Attachment #2: 0001-ubsan-do-return-check-with-fsanitize-unreachable.patch --] [-- Type: text/x-patch, Size: 4045 bytes --] From da268c4c1f9ac0a7eaa4d428791c3ed51cf0994d Mon Sep 17 00:00:00 2001 From: Jason Merrill <jason@redhat.com> Date: Wed, 15 Jun 2022 15:45:48 -0400 Subject: [PATCH] ubsan: do return check with -fsanitize=unreachable To: gcc-patches@gcc.gnu.org The current situation where we get less return checking with just -fsanitize=unreachable than no sanitize flags seems undesirable; we would get checking there except that we explicitly avoid emitting a __builtin_unreachable. The documented reason is that the use of BUILTIN_LOCATION makes the message confusing, so let's fix that. The !optimize check seems unneded since the PR104642 patch. gcc/cp/ChangeLog: * cp-gimplify.cc (cp_maybe_instrument_return): Pass the real location to the ubsan unreachable function. gcc/ChangeLog: * tree-cfg.cc (pass_warn_function_return::execute): Check for ubsan unreachable. gcc/testsuite/ChangeLog: * g++.dg/ubsan/return-8c.C: New test. --- gcc/cp/cp-gimplify.cc | 19 ++++++------------- gcc/testsuite/g++.dg/ubsan/return-8c.C | 16 ++++++++++++++++ gcc/tree-cfg.cc | 3 +++ 3 files changed, 25 insertions(+), 13 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ubsan/return-8c.C diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc index c05be833357..fcea9f8d0e0 100644 --- a/gcc/cp/cp-gimplify.cc +++ b/gcc/cp/cp-gimplify.cc @@ -1806,18 +1806,6 @@ cp_maybe_instrument_return (tree fndecl) || !targetm.warn_func_return (fndecl)) return; - if (!sanitize_flags_p (SANITIZE_RETURN, fndecl) - /* Don't add __builtin_unreachable () if not optimizing, it will not - improve any optimizations in that case, just break UB code. - Don't add it if -fsanitize=unreachable -fno-sanitize=return either, - UBSan covers this with ubsan_instrument_return above where sufficient - information is provided, while the __builtin_unreachable () below - if return sanitization is disabled will just result in hard to - understand runtime error without location. */ - && ((!optimize && !flag_unreachable_traps) - || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl))) - return; - tree t = DECL_SAVED_TREE (fndecl); while (t) { @@ -1864,7 +1852,12 @@ cp_maybe_instrument_return (tree fndecl) if (sanitize_flags_p (SANITIZE_RETURN, fndecl)) t = ubsan_instrument_return (loc); else - t = build_builtin_unreachable (BUILTINS_LOCATION); + { + /* Pass the real location to the ubsan function. */ + t = build_builtin_unreachable (loc); + /* But set BUILTINS_LOCATION for pass_warn_function_return. */ + SET_EXPR_LOCATION (t, BUILTINS_LOCATION); + } append_to_statement_list (t, p); } diff --git a/gcc/testsuite/g++.dg/ubsan/return-8c.C b/gcc/testsuite/g++.dg/ubsan/return-8c.C new file mode 100644 index 00000000000..828b24efa31 --- /dev/null +++ b/gcc/testsuite/g++.dg/ubsan/return-8c.C @@ -0,0 +1,16 @@ +// PR c++/104642 + +// -fsanitize=unreachable should catch missing return even without +// -fsanitize=return. + +// { dg-do run } +// { dg-shouldfail { *-*-* } } +// { dg-additional-options "-O -fsanitize=unreachable" } + +bool b; + +int f() { + if (b) return 42; +} // { dg-warning "-Wreturn-type" } + +int main() { f(); } diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index 734fdddbf97..647d98f0079 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -9551,10 +9551,13 @@ pass_warn_function_return::execute (function *fun) gimple *last = last_stmt (bb); const enum built_in_function ubsan_missing_ret = BUILT_IN_UBSAN_HANDLE_MISSING_RETURN; + const enum built_in_function ubsan_unreachable + = BUILT_IN_UBSAN_HANDLE_BUILTIN_UNREACHABLE; if (last && ((LOCATION_LOCUS (gimple_location (last)) == BUILTINS_LOCATION && (gimple_call_builtin_p (last, BUILT_IN_UNREACHABLE) + || gimple_call_builtin_p (last, ubsan_unreachable) || gimple_call_builtin_p (last, BUILT_IN_TRAP))) || gimple_call_builtin_p (last, ubsan_missing_ret))) { -- 2.27.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] ubsan: do return check with -fsanitize=unreachable 2022-06-22 4:04 ` Jason Merrill @ 2022-06-24 14:26 ` Jason Merrill 2022-06-27 15:44 ` Jakub Jelinek 1 sibling, 0 replies; 9+ messages in thread From: Jason Merrill @ 2022-06-24 14:26 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches On 6/22/22 00:04, Jason Merrill wrote: > On 6/20/22 16:16, Jason Merrill wrote: >> On 6/20/22 07:05, Jakub Jelinek wrote: >>> On Fri, Jun 17, 2022 at 05:20:02PM -0400, Jason Merrill wrote: >>>> Related to PR104642, the current situation where we get less return >>>> checking >>>> with just -fsanitize=unreachable than no sanitize flags seems >>>> undesirable; I >>>> propose that we do return checking when -fsanitize=unreachable. >>> >>> __builtin_unreachable itself (unless turned into trap or >>> __ubsan_handle_builtin_unreachable) is not any kind of return >>> checking, it >>> is just an optimization. >> >> Yes, but I'm talking about "when -fsanitize=unreachable". >> >>>> Looks like clang just traps on missing return if not >>>> -fsanitize=return, but >>>> the approach in this patch seems more helpful to me if we're already >>>> sanitizing other should-be-unreachable code. >>>> >>>> I'm assuming that the difference in treatment of >>>> SANITIZE_UNREACHABLE and >>>> SANITIZE_RETURN with regard to loop optimization is deliberate. >>> >>> return and unreachable are separate sanitizers and such silent one way >>> implication can have quite unexpected consequences, especially with >>> -fsanitize-trap=. >>> Say with -fsanitize=unreachable -fsanitize-trap=unreachable, both >>> current >>> trunk and clang will link without -lubsan, because the only enabled >>> UBSan >>> sanitizers use __builtin_trap () which doesn't need library. >>> With -fsanitize=unreachable silently meaning >>> -fsanitize=unreachable,return >>> the above would link in -lubsan, because while SANITIZE_UNREACHABLE uses >>> __builtin_trap, SANITIZE_RETURN doesn't. >>> Similarly, one has no_sanitize attribute, one could in certain function >>> __attribute__((no_sanitize ("unreachable"))) and because on the command >>> line using -fsanitize=unreachable assume other sanitizers aren't >>> enabled, >>> but the silent addition of return sanitizer would break that. >> >> Ah, true. How about this approach instead? > > Or, this approach relies on the PR104642 patch, and just fixes the line > number issue. This is less clear about the problem than using the > return ubsan library function, but avoids using one entry point to > implement the other sanitizer, if that's important. Ping? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] ubsan: do return check with -fsanitize=unreachable 2022-06-22 4:04 ` Jason Merrill 2022-06-24 14:26 ` Jason Merrill @ 2022-06-27 15:44 ` Jakub Jelinek 2022-06-29 16:42 ` Jason Merrill 1 sibling, 1 reply; 9+ messages in thread From: Jakub Jelinek @ 2022-06-27 15:44 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches On Wed, Jun 22, 2022 at 12:04:59AM -0400, Jason Merrill wrote: > On 6/20/22 16:16, Jason Merrill wrote: > > On 6/20/22 07:05, Jakub Jelinek wrote: > > > On Fri, Jun 17, 2022 at 05:20:02PM -0400, Jason Merrill wrote: > > > > Related to PR104642, the current situation where we get less > > > > return checking > > > > with just -fsanitize=unreachable than no sanitize flags seems > > > > undesirable; I > > > > propose that we do return checking when -fsanitize=unreachable. > > > > > > __builtin_unreachable itself (unless turned into trap or > > > __ubsan_handle_builtin_unreachable) is not any kind of return > > > checking, it > > > is just an optimization. > > > > Yes, but I'm talking about "when -fsanitize=unreachable". The usual case is that people just use -fsanitize=undefined and get both return and unreachable sanitization, for fall through into end of functions returning non-void done through return sanitization. In the rare case people use something different like -fsanitize=undefined -fno-sanitize=return or -fsanitize=unreachable etc., they presumably don't want the fall through from end of function diagnosed at runtime. I think the behavior we want is: 1) -fsanitize=return is on -> use ubsan_instrument_return (__ubsan_missing_return_data or __builtin_trap depending on -fsanitize-trap=return); otherwise 2) -funreachable-traps is on (from -O0/-Og by default or explicit), emit __builtin_trap; otherwise 3) -fsanitize=unreachable is on, not emit anything (__builtin_unreachable would be just an optimization, but user asked not to instrument returns, only unreachable, so honor user's decision and avoid confusion); otherwise 4) -O0 is on, not emit anything (__builtin_unreachable wouldn't be much of an optimization, just surprising and hard to debug effect); otherwise 5) emit __builtin_unreachable Current trunk with your PR104642 fix in implements 1), will do 2) only if -fsanitize=unreachable is not on, will do 3), will do 4) and 5). So, I'd change cp-gimplify.cc (cp_maybe_instrument_return), change: if (!sanitize_flags_p (SANITIZE_RETURN, fndecl) && ((!optimize && !flag_unreachable_traps) || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl))) return; to if (!sanitize_flags_p (SANITIZE_RETURN, fndecl) && !flag_unreachable_traps && (!optimize || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl))) return; and if (sanitize_flags_p (SANITIZE_RETURN, fndecl)) t = ubsan_instrument_return (loc); else t = build_builtin_unreachable (BUILTINS_LOCATION); to if (sanitize_flags_p (SANITIZE_RETURN, fndecl)) t = ubsan_instrument_return (loc); else if (flag_unreachable_traps) t = build_call_expr_loc (BUILTINS_LOCATION, builtin_decl_explicit (BUILT_IN_TRAP), 0); else t = build_builtin_unreachable (BUILTINS_LOCATION); Jakub ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] ubsan: do return check with -fsanitize=unreachable 2022-06-27 15:44 ` Jakub Jelinek @ 2022-06-29 16:42 ` Jason Merrill 2022-06-29 17:26 ` Jakub Jelinek 0 siblings, 1 reply; 9+ messages in thread From: Jason Merrill @ 2022-06-29 16:42 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches On 6/27/22 11:44, Jakub Jelinek wrote: > On Wed, Jun 22, 2022 at 12:04:59AM -0400, Jason Merrill wrote: >> On 6/20/22 16:16, Jason Merrill wrote: >>> On 6/20/22 07:05, Jakub Jelinek wrote: >>>> On Fri, Jun 17, 2022 at 05:20:02PM -0400, Jason Merrill wrote: >>>>> Related to PR104642, the current situation where we get less >>>>> return checking >>>>> with just -fsanitize=unreachable than no sanitize flags seems >>>>> undesirable; I >>>>> propose that we do return checking when -fsanitize=unreachable. >>>> >>>> __builtin_unreachable itself (unless turned into trap or >>>> __ubsan_handle_builtin_unreachable) is not any kind of return >>>> checking, it >>>> is just an optimization. >>> >>> Yes, but I'm talking about "when -fsanitize=unreachable". > > The usual case is that people just use -fsanitize=undefined > and get both return and unreachable sanitization, for fall through > into end of functions returning non-void done through return sanitization. > > In the rare case people use something different like > -fsanitize=undefined -fno-sanitize=return > or > -fsanitize=unreachable > etc., they presumably don't want the fall through from end of function > diagnosed at runtime. I disagree with this assumption for the second case; it seems much more likely to me that the user just wasn't thinking about needing to also mention return. Missing return is a logical subset of unreachable; if we sanitize all the other __builtin_unreachable introduced by the compiler, why in the world would we want to leave out this one that is such a frequent error? Full -fsanitize=undefined is much higher overhead than just -fsanitize=unreachable, which introduces no extra checks. And adding return checking to unreachable is essentially zero overhead. I can't imagine any reason to want to check unreachable points EXCEPT for missing return. > I think the behavior we want is: > 1) -fsanitize=return is on -> use ubsan_instrument_return > (__ubsan_missing_return_data or __builtin_trap depending on > -fsanitize-trap=return); otherwise > 2) -funreachable-traps is on (from -O0/-Og by default or explicit), > emit __builtin_trap; otherwise > 3) -fsanitize=unreachable is on, not emit anything (__builtin_unreachable > would be just an optimization, but user asked not to instrument returns, > only unreachable, so honor user's decision and avoid confusion); otherwise > 4) -O0 is on, not emit anything (__builtin_unreachable wouldn't be much > of an optimization, just surprising and hard to debug effect); otherwise > 5) emit __builtin_unreachable > > Current trunk with your PR104642 fix in implements 1), will do 2) > only if -fsanitize=unreachable is not on, will do 3), will do 4) and 5). > > So, I'd change cp-gimplify.cc (cp_maybe_instrument_return), change: > if (!sanitize_flags_p (SANITIZE_RETURN, fndecl) > && ((!optimize && !flag_unreachable_traps) > || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl))) > return; > to > if (!sanitize_flags_p (SANITIZE_RETURN, fndecl) > && !flag_unreachable_traps > && (!optimize || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl))) > return; > and > if (sanitize_flags_p (SANITIZE_RETURN, fndecl)) > t = ubsan_instrument_return (loc); > else > t = build_builtin_unreachable (BUILTINS_LOCATION); > to > if (sanitize_flags_p (SANITIZE_RETURN, fndecl)) > t = ubsan_instrument_return (loc); > else if (flag_unreachable_traps) > t = build_call_expr_loc (BUILTINS_LOCATION, > builtin_decl_explicit (BUILT_IN_TRAP), 0); > else > t = build_builtin_unreachable (BUILTINS_LOCATION); > > Jakub > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] ubsan: do return check with -fsanitize=unreachable 2022-06-29 16:42 ` Jason Merrill @ 2022-06-29 17:26 ` Jakub Jelinek 2022-07-05 20:54 ` Jason Merrill 0 siblings, 1 reply; 9+ messages in thread From: Jakub Jelinek @ 2022-06-29 17:26 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches On Wed, Jun 29, 2022 at 12:42:04PM -0400, Jason Merrill wrote: > > The usual case is that people just use -fsanitize=undefined > > and get both return and unreachable sanitization, for fall through > > into end of functions returning non-void done through return sanitization. > > > > In the rare case people use something different like > > -fsanitize=undefined -fno-sanitize=return > > or > > -fsanitize=unreachable > > etc., they presumably don't want the fall through from end of function > > diagnosed at runtime. > > I disagree with this assumption for the second case; it seems much more > likely to me that the user just wasn't thinking about needing to also > mention return. Missing return is a logical subset of unreachable; if we > sanitize all the other __builtin_unreachable introduced by the compiler, why > in the world would we want to leave out this one that is such a frequent > error? UBSan was initially implemented in LLVM and our -fsanitize= options try to match where possible what they do. And their behavior is too that return and unreachable are separate sanitizers, fallthrough from function return is sanitized only for the former, they apparently at -O0 implement something like -funreachable-traps (but not at -Og) and emit a trap there (regardless of -fsanitize=unreachable), for -O1 and higher they act as if non-sanitized __builtin_unreachable () is in there regardless of -fsanitize=unreachable. It would be strange to diverge from this without a strong reason. The fact that we use __builtin_unreachable for the function ends is just our implementation detail and when we'd report that to users, they'd just be confused on what's going on. With -fsanitize=return they are told what happens. > Full -fsanitize=undefined is much higher overhead than just > -fsanitize=unreachable, which introduces no extra checks. And adding return > checking to unreachable is essentially zero overhead. I can't imagine any > reason to want to check unreachable points EXCEPT for missing return. -fsanitize=unreachable isn't zero overhead, it will force evaluation of all the conditionals guarding it and preparation of arguments for it etc. Jakub ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFA] ubsan: do return check with -fsanitize=unreachable 2022-06-29 17:26 ` Jakub Jelinek @ 2022-07-05 20:54 ` Jason Merrill 0 siblings, 0 replies; 9+ messages in thread From: Jason Merrill @ 2022-07-05 20:54 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches On 6/29/22 13:26, Jakub Jelinek wrote: > On Wed, Jun 29, 2022 at 12:42:04PM -0400, Jason Merrill wrote: >>> The usual case is that people just use -fsanitize=undefined >>> and get both return and unreachable sanitization, for fall through >>> into end of functions returning non-void done through return sanitization. >>> >>> In the rare case people use something different like >>> -fsanitize=undefined -fno-sanitize=return >>> or >>> -fsanitize=unreachable >>> etc., they presumably don't want the fall through from end of function >>> diagnosed at runtime. >> >> I disagree with this assumption for the second case; it seems much more >> likely to me that the user just wasn't thinking about needing to also >> mention return. Missing return is a logical subset of unreachable; if we >> sanitize all the other __builtin_unreachable introduced by the compiler, why >> in the world would we want to leave out this one that is such a frequent >> error? > > UBSan was initially implemented in LLVM and our -fsanitize= options try to > match where possible what they do. > And their behavior is too that return and unreachable are separate > sanitizers, fallthrough from function return is sanitized only for the > former, they apparently at -O0 implement something like -funreachable-traps > (but not at -Og) and emit a trap there (regardless of > -fsanitize=unreachable), for -O1 and higher they act as if non-sanitized > __builtin_unreachable () is in there regardless of -fsanitize=unreachable. Hmm, does clang only sanitize explicit calls to __builtin_unreachable? > It would be strange to diverge from this without a strong reason. > The fact that we use __builtin_unreachable for the function ends is just our > implementation detail and when we'd report that to users, they'd just be > confused on what's going on. With -fsanitize=return they are told what > happens. > >> Full -fsanitize=undefined is much higher overhead than just >> -fsanitize=unreachable, which introduces no extra checks. And adding return >> checking to unreachable is essentially zero overhead. I can't imagine any >> reason to want to check unreachable points EXCEPT for missing return. > > -fsanitize=unreachable isn't zero overhead, it will force evaluation of all > the conditionals guarding it and preparation of arguments for it etc. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-07-05 20:55 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-17 21:20 [PATCH RFA] ubsan: do return check with -fsanitize=unreachable Jason Merrill 2022-06-20 11:05 ` Jakub Jelinek 2022-06-20 20:16 ` Jason Merrill 2022-06-22 4:04 ` Jason Merrill 2022-06-24 14:26 ` Jason Merrill 2022-06-27 15:44 ` Jakub Jelinek 2022-06-29 16:42 ` Jason Merrill 2022-06-29 17:26 ` Jakub Jelinek 2022-07-05 20:54 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).