Hi Jason, On 5 Nov 2024, at 3:30, Jason Merrill wrote:
> On 10/30/24 12:34 PM, Simon Martin wrote: >> Hi Jason, >> >> On 22 Oct 2024, at 17:07, Jason Merrill wrote: >> >>> On 10/17/24 10:30 AM, Simon Martin wrote: >>>> Hi, >>>> >>>> The issue reported in PR117129 is pretty similar to the one in >>>> PR117099, >>>> >>>> so here’s an updated version of the patch that fixes both issues, >>>> by >>>> ensuring that finish_return_expr sets current_function_return_value >>>> to >>>> error_mark_node in case of error with said return value. >>> >>>> permerror (input_location, "return-statement with no value, in " >>>> "function returning %qT", valtype); >>>> - /* Remember that this function did return. */ >>>> + /* Remember that this function returns an error. */ >>>> current_function_returns_value = 1; >>>> + retval = error_mark_node; >>> >>> We shouldn't do this if -fpermissive made the permerror into a >>> warning; let's just set current_function_return_value. >> Right, thanks for calling this out. The updated patch addresses this, >> >> and also adds an execution test case that was regressing with my >> initial >> version. Note that it relies on undefined behaviour, so I’m curious >> whether I should include it or not? > > I think let's include it, but add a comment that the return is UB in > case it breaks later. > >> @@ -11236,8 +11236,9 @@ check_return_expr (tree retval, bool >> *no_warning, bool *dangling) >> if (functype != error_mark_node) >> permerror (input_location, "return-statement with no value, in " >> "function returning %qT", valtype); >> - /* Remember that this function did return. */ >> + /* Remember that this function returns an error. */ > ... >> - /* If the conversion failed, treat this just like `return;'. >> */ >> + /* If the conversion failed, treat this just like >> + `return <ERROR>;'. */ > > Let's leave these comments alone, but comment on the > current_function_return_value assignments that we are suppressing NRV. > > OK with those comment adjustments. Thanks, amended accordingly and merged as r15-4959-gf31b72b75ef7cd. For the record I also renumbered the added tests since r15-4957-gb1d92aeb8583c8 added a crash77.C test already. Simon
