| Message ID | 20240901161740.73814-1-iain@sandoe.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | c++, coroutines: Instrument missing return_void UB. | expand |
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. > --- 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 <iain@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? > +#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? Jason
> On 4 Sep 2024, at 17:21, Jason Merrill <jason@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 <iain@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
On 9/4/24 4:00 PM, Iain Sandoe wrote: > > >> On 4 Sep 2024, at 17:21, Jason Merrill <jason@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 <iain@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
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 */ +#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. */ if (return_void) add_stmt (return_void); + else if (sanitize_flags_p (SANITIZE_RETURN, orig_fn_decl)) + add_stmt (ubsan_instrument_return (fn_start)); + else if (flag_unreachable_traps + && !sanitize_flags_p (SANITIZE_UNREACHABLE, orig_fn_decl)) + add_stmt (build_builtin_unreachable (fn_start)); TRY_STMTS (tcb) = pop_stmt_list (TRY_STMTS (tcb)); TRY_HANDLERS (tcb) = push_stmt_list (); /* Mimic what the parser does for the catch. */ @@ -4393,8 +4405,14 @@ cp_coroutine_transform::wrap_original_function_body () finish_expr_stmt (initial_await); /* Append the original function body. */ add_stmt (coroutine_body); + /* See comment above. */ if (return_void) add_stmt (return_void); + else if (sanitize_flags_p (SANITIZE_RETURN, orig_fn_decl)) + add_stmt (ubsan_instrument_return (fn_start)); + else if (flag_unreachable_traps + && !sanitize_flags_p (SANITIZE_UNREACHABLE, orig_fn_decl)) + add_stmt (build_builtin_unreachable (fn_start)); } /* co_return branches to the final_suspend label, so declare that now. */
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. tested on x86_64-darwin and powerpc64le-linux, apply to trunk? Opinions? thanks Iain --- 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 <iain@sandoe.co.uk> --- gcc/cp/coroutines.cc | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)