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.