On 9/4/24 4:00 PM, Iain Sandoe wrote:
On 4 Sep 2024, at 17:21, Jason Merrill <ja...@redhat.com> wrote:
On 9/1/24 12:17 PM, Iain Sandoe wrote:
This came up in discussion of an earlier patch.
I'm in two minds as to whether it's a good idea or not - the underlying
issue being that libubsan does not yet (AFAICT) have the concept of a
coroutine, so that the diagnostics are not very specific and might appear
strange (i.e. "execution reached the end of a value-returning function
without returning a value" which is a bit of an odd diagnostic for
a missing return_void ()).
OTOH one might argue that some diagnostic is better than silent UB .. but
I do not have cycles to address improving this in upstream libsanitizer ...
The logic used here is intended to match cp_maybe_instrument_return ()
although it's not 100% clear that that is doing exactly what the comment
says - since it does not distinguish between -fno-sanitize=return and
the case that the user simply did not specify it. So that
-fsanitize=unreachable is ignored for both fno-sanitize=return and the
unset case.
I think that's correct, what we care about is whether return sanitization is
enabled, not which flags were used to specify that.
OK.
--- 8< ---
[dcl.fct.def.coroutine] / 6 Note 1:
"If return_void is found, flowing off the end of a coroutine is equivalent
to a co_return with no operand. Otherwise, flowing off the end of a
coroutine results in undefined behavior."
Here we implement this as a check for sanitized returns and call the ubsan
instrumentation; if that is not enabled we mark this as unreachable (which
might trap depending on the target settings).
gcc/cp/ChangeLog:
* coroutines.cc
(cp_coroutine_transform::wrap_original_function_body): Instrument
the case where control flows off the end of a coroutine and the
user promise has no return_void entry.
Signed-off-by: Iain Sandoe <i...@sandoe.co.uk>
---
gcc/cp/coroutines.cc | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index e709d02b5f7..b67f4e3ef88 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -33,6 +33,9 @@ along with GCC; see the file COPYING3. If not see
#include "gcc-rich-location.h"
#include "hash-map.h"
#include "coroutines.h"
+#include "c-family/c-ubsan.h"
+#include "attribs.h" /* lookup_attribute */
I don't see any use of lookup_attribute?
It’s needed by asan.h
+#include "asan.h" /* sanitize_flags_p */
static bool coro_promise_type_found_p (tree, location_t);
@@ -4335,8 +4338,17 @@ cp_coroutine_transform::wrap_original_function_body ()
finish_expr_stmt (initial_await);
/* Append the original function body. */
add_stmt (coroutine_body);
+ /* Flowing off the end of a coroutine is equivalent to calling
+ promise.return_void () or is UB if the promise does not contain
+ that. Do not add an unreachable unless the user has asked for
+ checking of such cases. */
Let's mention that this is trying to parallel cp_maybe_instrument_return.
Or, actually, how about factoring out the actual statement building from that
function so we can use it here as well?
cp_maybe_instrument_return is local to cxx gimplify at moment
- do you have a preference for where the factored code might live?
Not particularly; I guess it might as well live in cp-gimplify.cc.
Jason
