Do not set nothrow flag on recursive function with stack checking

Hi,

when stack checking is enabled in Ada, it is supposed to be able to handle the 
case of a recursive function that does essentially nothing.  But in this case 
the IPA machinery will compute that the function is nothrow, which means that 
the Storage_Error exception cannot be caught by the caller.

Tested on x86-64/Linux, OK for the mainline?


2018-04-26  Eric Botcazou  <ebotcazou@adacore.com>

	* ipa-pure-const.c (check_call): In non-IPA mode, set can_throw flag
	for a recursive call that can throw if stack checking is enabled.
	(check_stmt): Fix formatting.
	(propagate_nothrow): Set can_throw for a recursive call if exceptions
	and stack checking are enabled.
	(pass_nothrow::execute): Do not set the nothrow flag if there is a
	recursive call that can throw and stack checking is enabled.


2018-04-26  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/stack_check4.adb: New test.

On Thu, Apr 26, 2018 at 4:29 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> when stack checking is enabled in Ada, it is supposed to be able to handle the
> case of a recursive function that does essentially nothing.  But in this case
> the IPA machinery will compute that the function is nothrow, which means that
> the Storage_Error exception cannot be caught by the caller.
>
> Tested on x86-64/Linux, OK for the mainline?

This looks not a generic enough fix to me - consider

void foo(void) { int a[100000]; a[0] = 1; a[99999] = 1; }
int main() { try { foo (); } catch (...) {} }

with -fnon-call-exceptions.  If we'd like to catch the SEGV from stack overflows
then your fix doesn't handle the non-recursive case nor the case where
-fstack-check
is not supplied.  But the compiler - when the accesses are in-range - will not
consider the accesses trapping.

So to me your attempt in fixing this isn't complete but a complete fix would
be quite pessimizing :/ (for -fnon-call-exceptions)

At least all this should be documented somewhere, that is, what to expect
when trying to catch stack faults in general with -fnon-call-exceptions
[-fstack-check].

Richard.

>
> 2018-04-26  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * ipa-pure-const.c (check_call): In non-IPA mode, set can_throw flag
>         for a recursive call that can throw if stack checking is enabled.
>         (check_stmt): Fix formatting.
>         (propagate_nothrow): Set can_throw for a recursive call if exceptions
>         and stack checking are enabled.
>         (pass_nothrow::execute): Do not set the nothrow flag if there is a
>         recursive call that can throw and stack checking is enabled.
>
>
> 2018-04-26  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/stack_check4.adb: New test.
>
> --
> Eric Botcazou

> This looks not a generic enough fix to me - consider
> 
> void foo(void) { int a[100000]; a[0] = 1; a[99999] = 1; }
> int main() { try { foo (); } catch (...) {} }
> 
> with -fnon-call-exceptions.  If we'd like to catch the SEGV from stack
> overflows then your fix doesn't handle the non-recursive case nor the case
> where -fstack-check is not supplied.

But -fstack-check is required to detect stack overflows...  Moreover the above 
testcase will be entirely optimized at -O so there is no stack overflow at -O.

> So to me your attempt in fixing this isn't complete but a complete fix would
> be quite pessimizing :/ (for -fnon-call-exceptions)

Then let's at least fix the recursive case since it's simple and cheap.  But I 
can indeed key this on -fnon-call-exceptions explicitly.

> At least all this should be documented somewhere, that is, what to expect
> when trying to catch stack faults in general with -fnon-call-exceptions
> [-fstack-check].

It's already documented that you need -fstack-check to detect stack overflows.
But I can add a blurb to the -fnon-call-exceptions entry about it.

Revised patch attached.

On Sat, Apr 28, 2018 at 11:20 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> This looks not a generic enough fix to me - consider
>>
>> void foo(void) { int a[100000]; a[0] = 1; a[99999] = 1; }
>> int main() { try { foo (); } catch (...) {} }
>>
>> with -fnon-call-exceptions.  If we'd like to catch the SEGV from stack
>> overflows then your fix doesn't handle the non-recursive case nor the case
>> where -fstack-check is not supplied.
>
> But -fstack-check is required to detect stack overflows...  Moreover the above
> testcase will be entirely optimized at -O so there is no stack overflow at -O.
>
>> So to me your attempt in fixing this isn't complete but a complete fix would
>> be quite pessimizing :/ (for -fnon-call-exceptions)
>
> Then let's at least fix the recursive case since it's simple and cheap.  But I
> can indeed key this on -fnon-call-exceptions explicitly.
>
>> At least all this should be documented somewhere, that is, what to expect
>> when trying to catch stack faults in general with -fnon-call-exceptions
>> [-fstack-check].
>
> It's already documented that you need -fstack-check to detect stack overflows.
> But I can add a blurb to the -fnon-call-exceptions entry about it.
>
> Revised patch attached.

Is the propagate_nothrow hunk really necessary since you set ->can_throw = true
in local analysis?  Again this not only handles only recursion but also only
direct recursion.  I wonder if there's a way to prove (or at least estimate with
good confidence) that a function does _not_ need local stack space?  Thus
for -fstack-check -fnon-call-exceptions consider all functions not
marked explicitely
as can_throw unless we "prove" the opposite?  Or consider all non-leaf functions
as possibly throwing that way?

I'm not against your patch but I think that this kind of limitations
need to be documented.

Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi     (revision 259642)
+++ doc/invoke.texi     (working copy)
@@ -12812,6 +12812,9 @@ not exist everywhere.  Moreover, it only
 instructions to throw exceptions, i.e.@: memory references or floating-point
 instructions.  It does not allow exceptions to be thrown from
 arbitrary signal handlers such as @code{SIGALRM}.
+This option must be specified if you enable @option{-fstack-check} and
+want stack overflows to throw exceptions.  Note that this again requires
+platform-specific runtime support.


I'd phrase it as "If you want to handle stack overflows as exceptions
you need to
enable @option{-fstack-check} in addition to this option.  Support for this is
experimental, some stack overflows might not be catchable."

Honza, do you have any suggestions here for the issue of detecting possible
recursion as opposted to just direct recursion?

Richard.

> --
> Eric Botcazou

> 
> Honza, do you have any suggestions here for the issue of detecting possible
> recursion as opposted to just direct recursion?

Well, you can look for strongly connected components but you must assume that
every call out of the current unit that is not leaf may call any function that
is externally visible or has address taken. So basically every function that may
lead to external call is possibly recursive because you can assume that every
function is reachable (with exception of main).

This will rule out many nothrow candidates but I do not see how that can be
strenghtened.

Honza
> 
> Richard.
> 
> > --
> > Eric Botcazou