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

[ian at airs dot com]

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

--- Comment #26 from Ian Lance Taylor <ian at airs dot com> ---
(In reply to Dominik Vogt from comment #23)
> 
> 1) We need to call __builtin_extract_return_address(retaddr) in
> __go_set_defer_retaddr() too.  (On s390 (31 bit) this is necessary to remove
> the most significant bit of the address which indicates the addressing mode.)
> 
> I.e.
> 
> --snip--
> -    g->defer->__retaddr = retaddr;
> +    g->defer->__retaddr = __builtin_extract_return_addr (retaddr);
> --snip--

Will do.


> 2) The new patch does not compile for me:
> 
> --
> In file included from ../../../libgo/runtime/go-check-interface.c:8:0:
> ../../../libgo/runtime/go-panic.h:43:13: error: conflicting types for
> ‘__go_makefunc_can_recover’
>  extern void __go_makefunc_can_recover (void *retaddr);
>              ^
> In file included from ../../../libgo/runtime/go-check-interface.c:7:0:
> ../../../libgo/runtime/runtime.h:845:9: note: previous declaration of
> ‘__go_makefunc_can_recover’ was here
>  void    __go_makefunc_can_recover (const void *);
>          ^
> --
> 
> In runtime.h, __go_makefunc_can_recover still has a "const" argument:
> 
> --snip--
> -void    __go_makefunc_can_recover (const void *);
> +void    __go_makefunc_can_recover (void *);
> --snip--

I think that must be a local change in your tree.  In my tree runtime.h does
not declare __go_makefunc_can_recover.


> 3) Couldn't this be written more efficiently by passing a flag?
> 
> +  /* If we are called from __go_makefunc_can_recover, then we need to
> +     look one level higher.  */
> +  if (locs[0].function.len > 0
> +      && __builtin_strcmp ((const char *) locs[0].function.str,
> +			   "__go_makefunc_can_recover") == 0)
> 
> E.g.
> 
>   _Bool __go_can_recover (void *retaddr, _Bool
> called_by_makefunc_can_recover)
>   {
>     ...
>     if (called_by_makefunc_can_recover)
>       { do something }
>     else
>       { do something else }
>     ...
>   }

Yes, but I would rather do that later if it seems useful.  It seems silly to
change the compiler to always pass an extra argument that will always be false.
 Introducing a new function called by both __go_can_recover and
__go_makefunc_can_recover changes the stack layout depending on the compiler's
inlining choices, so must be treated with care.  And it's worth remembering
that this case never ever happens outside of tests.  It only happens when
somebody constructs a function using reflect.MakeFunc and then defers a call to
that function.  That is a bizarre way to code and I am confident that
absolutely no real Go code does it.  Making that case slightly less efficient
is not important.


> 4) With the suggested changes from 1 and 2 above:
> 
> s390x (64 bit):
> 
> * PASS: test/recover.go
> * the test from #4 in my earlier posting works as expected
> * my private defer/recover/panic testsuite works as expected
> 
> s390 (31 bit):
> 
> * PASS: test/recover.go
> * the test from #4 in my earlier posting works as expected
> * my private defer/recover/panic testsuite works as expected

Thanks for testing.


> 5) I find the assumption in the loop at the end of __go_can_recover() that
> if a caller's name begins with "__go_" that means the panic can be
> recovered, a bit hairy.  Even if it is with the current libgo, an unrelated
> change elsewhere could break this logic.

I agree that it's imperfect.  However, it's fairly difficult to construct a
case that causes the wrong thing to happen.  No Go function can ever start with
__go.  Very little code in libgo calls a function written in Go; it's easy to
find such code because it must call __go_set_closure (the defer thunks are a
special case that are known to have no closure).  So it can only happen if
somebody writes a Go function that calls recover, and then passes it to C code,
and that C code names a function starting with "__go_" and then calls the Go
function.  And this has to happen from a deferred function called while there
is a panic in progress.  The result will be that the function called by the C
code will incorrectly recover the panic.