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