> 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?
thanks
Iain
>
> Jason
