https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60406
--- Comment #16 from Ian Lance Taylor <ian at airs dot com> ---
>> I'm not really happy with Dominik's patch because 1) it doesn't work when
>> configuring with --enable-sjlj-exceptions;
>
> Why is that important?
It's not very important but it's still a point to consider. Some targets
default to SJLJ exceptions, albeit not very important ones.
>> 2) the current code almost always works, even on S/390, but the patch
>> forces us to do a lookup in the FDE table every time we call recover.
>
> The current code works unreliably as s390 uses memcopy to copy call
> arguments to the stack. The control flow introduced by the function
> call triggers basic block reordering that may result in anything.
Sure, I understand, and it can obviously cause a false negative: some cases
that should recover will fail to do so. However, I don't see any way that it
can ever cause a false positive: I don't see any way that it can cause recover
to succeed when it should not.
> * On systems that "use a leading underscore on symbol names", the test
> for functions beginning with "__go_" or "_go_" would yield "true" from user
> functions named "_go_..." (because the system adds one '_' and the patch
> strips it).
Yes. We are already going to have trouble on such systems. Really the library
needs to learn which systems use a leading underscore and which do not. This
is actually available as __USER_LABEL_PREFIX__, and we should use that.
> * 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'll have to figure out why.
> * 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. In the normal
case, where recover is permitted and the existing code works, we save the FDE
lookup.
> 2) The current checks for "return address + 16" may point into a
> different function, allowing recover() in weird situations.
It's a potential problem but I'm not too worried about it.
> "The return value of recover is nil if any of the following conditions holds:
> ...
> *recover was not called directly by a deferred function."
>
> According to the spec, the following code should recover the panic but
> does not:
>
> func main() { defer foo(); panic("..."); }
> func foo() { defer bar(); }
> func bar() { recover(); }
>
> Note that this is also also "broken" in Golang (well, at least in the old
> version that comes with Ubuntu). This may be an effect of imprecise
> wording of the spec.
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. In your example, the defer of bar
happens after the call to panic. The reason Go works this way is to that the
deferred function foo can itself call a function that panics and recovers
without that function being confused by the earlier panic, one that it may not
know anything about.
> 4) __go_can_recover assumes that any call through libffi is allowed
> to recover.
Thanks for the example. Does your patch fix this problem?
