[Bug go/60406] recover.go: test13reflect2 test failure

["vogt at linux dot vnet.ibm.com" <gcc-bugzilla@gcc.gnu.org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60406

--- Comment #17 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
>> * Wouldn't the new patch re-introduce the bug that
>>
>>   func foo(n int) {
>>     if (n == 0) { recover(); } else { foo(0); }
>>   }
>>   func main() {
>>     defer foo(1)
>>     panic("...")
>>   }
>> 
>>   would recover although it should not?
>
> Hmmm, I hadn't fully internalized that issue.  Your new
> withoutRecoverRecursive test doesn't fail for me on x86_64.

I think you have implicitly fixed this issue by splitting functions that call
recover() into two parts (i.e. main.foo and main.foo$recover).  So recursive
calls originate from the ...$recover function and never match the return
address check.  Well, maybe it was only a problem with tail recursion, i.e.

  func foo(n int) int {
    if (n == 0) { recover(); return 0; }
    return foo(0)
  }

Would be optimized to a loop, removing the function call, and then the return
address check would trigger.  That's not possible with two function approach. 
But if there's another criterion to allow recover that simply depends on the
caller's name the problem might reappear.

>> * The code is even more expensive than the approach I had chosen because
>> now it needs to fetch a two level backtrace instead of just one level
>> (and probably each level is more expensive than the one 
>> _Unwind_FindEnclosingFunc()).
>
> Yes, but the expensive case only happens in the rare cases where
> either recover should not work or when the existing code has a
> false negative.

Hm, so the patch penalises platforms that cannot deal with the 16 byte window?

>>   func main() { defer foo(); panic("..."); }
>>   func foo() { defer bar(); }
>>   func bar() { recover(); }
>
> In this case, the call to recover in bar is supposed to return nil;
> it should not recover the panic.  If you read the paragraph before
> the one you quote, you will see that recover only returns non-nil
> if it was called by a function that was deferred before the call to
> panic.

I've read it but cannot see anything that would disallow recovery in this
situation.  What exactly do you mean?

>> 4) __go_can_recover assumes that any call through libffi is allowed
>> to recover.
>
> Thanks for the example.  Does your patch fix this problem?

No, I just became aware of the problem when reviewing the code last week.