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.From da268c4c1f9ac0a7eaa4d428791c3ed51cf0994d 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; 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
