From 439c645fa5197ccbfcb6fbbeda5772a8581c3a7e Mon Sep 17 00:00:00 2001 From: Jason Merrill 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