Fix ICE after sorry for big stack arguments (PR 84964)

Message ID 871sgdzv2y.fsf@linaro.org
State New
Headers show
Series
  • Fix ICE after sorry for big stack arguments (PR 84964)
Related show

Commit Message

Richard Sandiford March 21, 2018, 8:33 a.m.
After the sorry we'd skip storing the argument, but that just creates an
inconsistency later when we try to deallocate the arguments.  This used to
"work" because pending_stack_adjust and stack_pointer_delta were int rather
than HWI, so 1<<33 got truncated to 0.

It's not easy to back out at this point because there's so much global
state floating around.  One option I tried was to put the sorry after:

      unadjusted_args_size
	= compute_argument_block_size (reg_parm_stack_space,
				       &adjusted_args_size,
				       fndecl, fntype,
				       (pass == 0 ? 0
					: preferred_stack_boundary));

and then zero the argument sizes and num_actual.  That avoids the
ICE too, but who knows what other problems it creates.

In the end it seemed easier just to stop.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Richard


2018-03-21  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	PR error-recovery/84964
	* diagnostic-core.h (fatal_sorry): Declare.
	* diagnostic.c (fatal_sorry): New function.
	* calls.c (expand_call): Use it instead of sorry.

gcc/testsuite/
	PR error-recovery/84964
	* g++.dg/diagnostic/pr84964.C: New test.

Comments

Richard Biener March 21, 2018, 9:53 a.m. | #1
On Wed, Mar 21, 2018 at 9:33 AM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> After the sorry we'd skip storing the argument, but that just creates an

> inconsistency later when we try to deallocate the arguments.  This used to

> "work" because pending_stack_adjust and stack_pointer_delta were int rather

> than HWI, so 1<<33 got truncated to 0.

>

> It's not easy to back out at this point because there's so much global

> state floating around.  One option I tried was to put the sorry after:

>

>       unadjusted_args_size

>         = compute_argument_block_size (reg_parm_stack_space,

>                                        &adjusted_args_size,

>                                        fndecl, fntype,

>                                        (pass == 0 ? 0

>                                         : preferred_stack_boundary));

>

> and then zero the argument sizes and num_actual.  That avoids the

> ICE too, but who knows what other problems it creates.

>

> In the end it seemed easier just to stop.

>

> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

>

> Richard

>

>

> 2018-03-21  Richard Sandiford  <richard.sandiford@linaro.org>

>

> gcc/

>         PR error-recovery/84964

>         * diagnostic-core.h (fatal_sorry): Declare.

>         * diagnostic.c (fatal_sorry): New function.

>         * calls.c (expand_call): Use it instead of sorry.

>

> gcc/testsuite/

>         PR error-recovery/84964

>         * g++.dg/diagnostic/pr84964.C: New test.

>

> Index: gcc/diagnostic-core.h

> ===================================================================

> --- gcc/diagnostic-core.h       2018-03-01 08:20:43.590534306 +0000

> +++ gcc/diagnostic-core.h       2018-03-21 08:31:34.635677798 +0000

> @@ -87,6 +87,8 @@ extern bool permerror (location_t, const

>  extern bool permerror (rich_location *, const char *,

>                                    ...) ATTRIBUTE_GCC_DIAG(2,3);

>  extern void sorry (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2);

> +extern void fatal_sorry (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2)

> +    ATTRIBUTE_NORETURN;

>  extern void inform (location_t, const char *, ...) ATTRIBUTE_GCC_DIAG(2,3);

>  extern void inform (rich_location *, const char *, ...) ATTRIBUTE_GCC_DIAG(2,3);

>  extern void inform_n (location_t, unsigned HOST_WIDE_INT, const char *,

> Index: gcc/diagnostic.c

> ===================================================================

> --- gcc/diagnostic.c    2018-03-01 08:20:43.589534337 +0000

> +++ gcc/diagnostic.c    2018-03-21 08:31:34.635677798 +0000

> @@ -1407,6 +1407,20 @@ sorry (const char *gmsgid, ...)

>    va_end (ap);

>  }

>

> +/* Same, but stop compilation immediately.  */

> +

> +void

> +fatal_sorry (const char *gmsgid, ...)

> +{

> +  va_list ap;

> +  va_start (ap, gmsgid);

> +  rich_location richloc (line_table, input_location);

> +  diagnostic_impl (&richloc, -1, gmsgid, &ap, DK_SORRY);

> +  va_end (ap);

> +

> +  exit (FATAL_EXIT_CODE);


This is somewhat not in style of the other "fatal" handlers.  I guess
you don't get "sorry: " when using DK_FATAL, but ...

> +}

> +

>  /* Return true if an error or a "sorry" has been seen.  Various

>     processing is disabled after errors.  */

>  bool

> Index: gcc/calls.c

> ===================================================================

> --- gcc/calls.c 2018-03-17 08:30:20.937100705 +0000

> +++ gcc/calls.c 2018-03-21 08:31:34.635677798 +0000

> @@ -4052,13 +4052,10 @@ expand_call (tree exp, rtx target, int i

>               rtx_insn *before_arg = get_last_insn ();

>

>               /* We don't allow passing huge (> 2^30 B) arguments

> -                by value.  It would cause an overflow later on.  */

> +                by value.  It would cause an overflow later on.  */

>               if (constant_lower_bound (adjusted_args_size.constant)

>                   >= (1 << (HOST_BITS_PER_INT - 2)))

> -               {

> -                 sorry ("passing too large argument on stack");

> -                 continue;

> -               }

> +               fatal_sorry ("passing too large argument on stack");


... why not use fatal_error here?  It doesn't seem to be something
that is just not implemented but impossible to implement?

So, OK with just using fatal_error here (please add a comment why
we're not continuing).

Richard.

>

>               if (store_one_arg (&args[i], argblock, flags,

>                                  adjusted_args_size.var != 0,

> Index: gcc/testsuite/g++.dg/diagnostic/pr84964.C

> ===================================================================

> --- /dev/null   2018-03-17 08:19:33.716019995 +0000

> +++ gcc/testsuite/g++.dg/diagnostic/pr84964.C   2018-03-21 08:31:34.635677798 +0000

> @@ -0,0 +1,6 @@

> +/* { dg-do compile { target lp64 } } */

> +

> +struct a {

> +  short b : 1ULL << 33; /* { dg-warning "width of 'a::b' exceeds its type" } */

> +};

> +void c(...) { c(a()); } /* { dg-message "sorry, unimplemented: passing too large argument on stack" } */
Richard Sandiford March 21, 2018, 10:48 a.m. | #2
Richard Biener <richard.guenther@gmail.com> writes:
> On Wed, Mar 21, 2018 at 9:33 AM, Richard Sandiford

> <richard.sandiford@linaro.org> wrote:

>> Index: gcc/diagnostic.c

>> ===================================================================

>> --- gcc/diagnostic.c    2018-03-01 08:20:43.589534337 +0000

>> +++ gcc/diagnostic.c    2018-03-21 08:31:34.635677798 +0000

>> @@ -1407,6 +1407,20 @@ sorry (const char *gmsgid, ...)

>>    va_end (ap);

>>  }

>>

>> +/* Same, but stop compilation immediately.  */

>> +

>> +void

>> +fatal_sorry (const char *gmsgid, ...)

>> +{

>> +  va_list ap;

>> +  va_start (ap, gmsgid);

>> +  rich_location richloc (line_table, input_location);

>> +  diagnostic_impl (&richloc, -1, gmsgid, &ap, DK_SORRY);

>> +  va_end (ap);

>> +

>> +  exit (FATAL_EXIT_CODE);

>

> This is somewhat not in style of the other "fatal" handlers.  I guess

> you don't get "sorry: " when using DK_FATAL, but ...


I didn't add another DK_* because I don't think we want a different prefix
(unliked "error:" vs. "fatal error:").

>> +}

>> +

>>  /* Return true if an error or a "sorry" has been seen.  Various

>>     processing is disabled after errors.  */

>>  bool

>> Index: gcc/calls.c

>> ===================================================================

>> --- gcc/calls.c 2018-03-17 08:30:20.937100705 +0000

>> +++ gcc/calls.c 2018-03-21 08:31:34.635677798 +0000

>> @@ -4052,13 +4052,10 @@ expand_call (tree exp, rtx target, int i

>>               rtx_insn *before_arg = get_last_insn ();

>>

>>               /* We don't allow passing huge (> 2^30 B) arguments

>> -                by value.  It would cause an overflow later on.  */

>> +                by value.  It would cause an overflow later on.  */

>>               if (constant_lower_bound (adjusted_args_size.constant)

>>                   >= (1 << (HOST_BITS_PER_INT - 2)))

>> -               {

>> -                 sorry ("passing too large argument on stack");

>> -                 continue;

>> -               }

>> +               fatal_sorry ("passing too large argument on stack");

>

> ... why not use fatal_error here?  It doesn't seem to be something

> that is just not implemented but impossible to implement?


It's HOST_BITS_PER_INT rather than _WIDE_INT, so I think we could
implement it if we were tighter with types.  E.g. this PR occurred
because we went from int to HWI for some things, which should have
fixed part of the problem.  There might still be other uses of
"int" that need to change though.

Thanks,
Richard

> So, OK with just using fatal_error here (please add a comment why

> we're not continuing).
Richard Biener March 21, 2018, 10:54 a.m. | #3
On Wed, Mar 21, 2018 at 11:48 AM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:

>> On Wed, Mar 21, 2018 at 9:33 AM, Richard Sandiford

>> <richard.sandiford@linaro.org> wrote:

>>> Index: gcc/diagnostic.c

>>> ===================================================================

>>> --- gcc/diagnostic.c    2018-03-01 08:20:43.589534337 +0000

>>> +++ gcc/diagnostic.c    2018-03-21 08:31:34.635677798 +0000

>>> @@ -1407,6 +1407,20 @@ sorry (const char *gmsgid, ...)

>>>    va_end (ap);

>>>  }

>>>

>>> +/* Same, but stop compilation immediately.  */

>>> +

>>> +void

>>> +fatal_sorry (const char *gmsgid, ...)

>>> +{

>>> +  va_list ap;

>>> +  va_start (ap, gmsgid);

>>> +  rich_location richloc (line_table, input_location);

>>> +  diagnostic_impl (&richloc, -1, gmsgid, &ap, DK_SORRY);

>>> +  va_end (ap);

>>> +

>>> +  exit (FATAL_EXIT_CODE);

>>

>> This is somewhat not in style of the other "fatal" handlers.  I guess

>> you don't get "sorry: " when using DK_FATAL, but ...

>

> I didn't add another DK_* because I don't think we want a different prefix

> (unliked "error:" vs. "fatal error:").

>

>>> +}

>>> +

>>>  /* Return true if an error or a "sorry" has been seen.  Various

>>>     processing is disabled after errors.  */

>>>  bool

>>> Index: gcc/calls.c

>>> ===================================================================

>>> --- gcc/calls.c 2018-03-17 08:30:20.937100705 +0000

>>> +++ gcc/calls.c 2018-03-21 08:31:34.635677798 +0000

>>> @@ -4052,13 +4052,10 @@ expand_call (tree exp, rtx target, int i

>>>               rtx_insn *before_arg = get_last_insn ();

>>>

>>>               /* We don't allow passing huge (> 2^30 B) arguments

>>> -                by value.  It would cause an overflow later on.  */

>>> +                by value.  It would cause an overflow later on.  */

>>>               if (constant_lower_bound (adjusted_args_size.constant)

>>>                   >= (1 << (HOST_BITS_PER_INT - 2)))

>>> -               {

>>> -                 sorry ("passing too large argument on stack");

>>> -                 continue;

>>> -               }

>>> +               fatal_sorry ("passing too large argument on stack");

>>

>> ... why not use fatal_error here?  It doesn't seem to be something

>> that is just not implemented but impossible to implement?

>

> It's HOST_BITS_PER_INT rather than _WIDE_INT, so I think we could

> implement it if we were tighter with types.  E.g. this PR occurred

> because we went from int to HWI for some things, which should have

> fixed part of the problem.  There might still be other uses of

> "int" that need to change though.


Ah, missed the BITS part.  I wonder if we can get away with simply
setting adjusted_args_size.constant to some small constant?  For
bits we generally need offset_ints, just HWI isn't enough anyway.

Richard.

> Thanks,

> Richard

>

>> So, OK with just using fatal_error here (please add a comment why

>> we're not continuing).
Richard Sandiford March 21, 2018, 11:16 a.m. | #4
Richard Biener <richard.guenther@gmail.com> writes:
> On Wed, Mar 21, 2018 at 11:48 AM, Richard Sandiford

> <richard.sandiford@linaro.org> wrote:

>> Richard Biener <richard.guenther@gmail.com> writes:

>>> On Wed, Mar 21, 2018 at 9:33 AM, Richard Sandiford

>>> <richard.sandiford@linaro.org> wrote:

>>>> Index: gcc/diagnostic.c

>>>> ===================================================================

>>>> --- gcc/diagnostic.c    2018-03-01 08:20:43.589534337 +0000

>>>> +++ gcc/diagnostic.c    2018-03-21 08:31:34.635677798 +0000

>>>> @@ -1407,6 +1407,20 @@ sorry (const char *gmsgid, ...)

>>>>    va_end (ap);

>>>>  }

>>>>

>>>> +/* Same, but stop compilation immediately.  */

>>>> +

>>>> +void

>>>> +fatal_sorry (const char *gmsgid, ...)

>>>> +{

>>>> +  va_list ap;

>>>> +  va_start (ap, gmsgid);

>>>> +  rich_location richloc (line_table, input_location);

>>>> +  diagnostic_impl (&richloc, -1, gmsgid, &ap, DK_SORRY);

>>>> +  va_end (ap);

>>>> +

>>>> +  exit (FATAL_EXIT_CODE);

>>>

>>> This is somewhat not in style of the other "fatal" handlers.  I guess

>>> you don't get "sorry: " when using DK_FATAL, but ...

>>

>> I didn't add another DK_* because I don't think we want a different prefix

>> (unliked "error:" vs. "fatal error:").

>>

>>>> +}

>>>> +

>>>>  /* Return true if an error or a "sorry" has been seen.  Various

>>>>     processing is disabled after errors.  */

>>>>  bool

>>>> Index: gcc/calls.c

>>>> ===================================================================

>>>> --- gcc/calls.c 2018-03-17 08:30:20.937100705 +0000

>>>> +++ gcc/calls.c 2018-03-21 08:31:34.635677798 +0000

>>>> @@ -4052,13 +4052,10 @@ expand_call (tree exp, rtx target, int i

>>>>               rtx_insn *before_arg = get_last_insn ();

>>>>

>>>>               /* We don't allow passing huge (> 2^30 B) arguments

>>>> -                by value.  It would cause an overflow later on.  */

>>>> +                by value.  It would cause an overflow later on.  */

>>>>               if (constant_lower_bound (adjusted_args_size.constant)

>>>>                   >= (1 << (HOST_BITS_PER_INT - 2)))

>>>> -               {

>>>> -                 sorry ("passing too large argument on stack");

>>>> -                 continue;

>>>> -               }

>>>> +               fatal_sorry ("passing too large argument on stack");

>>>

>>> ... why not use fatal_error here?  It doesn't seem to be something

>>> that is just not implemented but impossible to implement?

>>

>> It's HOST_BITS_PER_INT rather than _WIDE_INT, so I think we could

>> implement it if we were tighter with types.  E.g. this PR occurred

>> because we went from int to HWI for some things, which should have

>> fixed part of the problem.  There might still be other uses of

>> "int" that need to change though.

>

> Ah, missed the BITS part.  I wonder if we can get away with simply

> setting adjusted_args_size.constant to some small constant?  For

> bits we generally need offset_ints, just HWI isn't enough anyway.


This is a byte quantity, so HWI should be OK.  We're just using
HOST_BITS_PER_INT to calculate the range of int on the host.

I think we'd run into other problems if the accumulated size didn't
match the sizes of the individual arguments.  That's why setting the
sizes to zero and the number of arguments to zero seemed "safer".
But I don't think anyone's really thought about what else could go wrong
if we make arbitrary changes like that.  Just bailing out seems better.

Thanks,
Richard
Richard Biener March 21, 2018, 12:01 p.m. | #5
On Wed, Mar 21, 2018 at 12:16 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:

>> On Wed, Mar 21, 2018 at 11:48 AM, Richard Sandiford

>> <richard.sandiford@linaro.org> wrote:

>>> Richard Biener <richard.guenther@gmail.com> writes:

>>>> On Wed, Mar 21, 2018 at 9:33 AM, Richard Sandiford

>>>> <richard.sandiford@linaro.org> wrote:

>>>>> Index: gcc/diagnostic.c

>>>>> ===================================================================

>>>>> --- gcc/diagnostic.c    2018-03-01 08:20:43.589534337 +0000

>>>>> +++ gcc/diagnostic.c    2018-03-21 08:31:34.635677798 +0000

>>>>> @@ -1407,6 +1407,20 @@ sorry (const char *gmsgid, ...)

>>>>>    va_end (ap);

>>>>>  }

>>>>>

>>>>> +/* Same, but stop compilation immediately.  */

>>>>> +

>>>>> +void

>>>>> +fatal_sorry (const char *gmsgid, ...)

>>>>> +{

>>>>> +  va_list ap;

>>>>> +  va_start (ap, gmsgid);

>>>>> +  rich_location richloc (line_table, input_location);

>>>>> +  diagnostic_impl (&richloc, -1, gmsgid, &ap, DK_SORRY);

>>>>> +  va_end (ap);

>>>>> +

>>>>> +  exit (FATAL_EXIT_CODE);

>>>>

>>>> This is somewhat not in style of the other "fatal" handlers.  I guess

>>>> you don't get "sorry: " when using DK_FATAL, but ...

>>>

>>> I didn't add another DK_* because I don't think we want a different prefix

>>> (unliked "error:" vs. "fatal error:").

>>>

>>>>> +}

>>>>> +

>>>>>  /* Return true if an error or a "sorry" has been seen.  Various

>>>>>     processing is disabled after errors.  */

>>>>>  bool

>>>>> Index: gcc/calls.c

>>>>> ===================================================================

>>>>> --- gcc/calls.c 2018-03-17 08:30:20.937100705 +0000

>>>>> +++ gcc/calls.c 2018-03-21 08:31:34.635677798 +0000

>>>>> @@ -4052,13 +4052,10 @@ expand_call (tree exp, rtx target, int i

>>>>>               rtx_insn *before_arg = get_last_insn ();

>>>>>

>>>>>               /* We don't allow passing huge (> 2^30 B) arguments

>>>>> -                by value.  It would cause an overflow later on.  */

>>>>> +                by value.  It would cause an overflow later on.  */

>>>>>               if (constant_lower_bound (adjusted_args_size.constant)

>>>>>                   >= (1 << (HOST_BITS_PER_INT - 2)))

>>>>> -               {

>>>>> -                 sorry ("passing too large argument on stack");

>>>>> -                 continue;

>>>>> -               }

>>>>> +               fatal_sorry ("passing too large argument on stack");

>>>>

>>>> ... why not use fatal_error here?  It doesn't seem to be something

>>>> that is just not implemented but impossible to implement?

>>>

>>> It's HOST_BITS_PER_INT rather than _WIDE_INT, so I think we could

>>> implement it if we were tighter with types.  E.g. this PR occurred

>>> because we went from int to HWI for some things, which should have

>>> fixed part of the problem.  There might still be other uses of

>>> "int" that need to change though.

>>

>> Ah, missed the BITS part.  I wonder if we can get away with simply

>> setting adjusted_args_size.constant to some small constant?  For

>> bits we generally need offset_ints, just HWI isn't enough anyway.

>

> This is a byte quantity, so HWI should be OK.  We're just using

> HOST_BITS_PER_INT to calculate the range of int on the host.


Ok...

> I think we'd run into other problems if the accumulated size didn't

> match the sizes of the individual arguments.  That's why setting the

> sizes to zero and the number of arguments to zero seemed "safer".

> But I don't think anyone's really thought about what else could go wrong

> if we make arbitrary changes like that.  Just bailing out seems better.


Fine.  Then sorry () plus fatal_error () with a comment still sounds best here
(if we care at all about error-recovery issues - care to "paper over
them" instead
of fixing them for real, whatever that means)

Richard.

> Thanks,

> Richard

Patch

Index: gcc/diagnostic-core.h
===================================================================
--- gcc/diagnostic-core.h	2018-03-01 08:20:43.590534306 +0000
+++ gcc/diagnostic-core.h	2018-03-21 08:31:34.635677798 +0000
@@ -87,6 +87,8 @@  extern bool permerror (location_t, const
 extern bool permerror (rich_location *, const char *,
 				   ...) ATTRIBUTE_GCC_DIAG(2,3);
 extern void sorry (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2);
+extern void fatal_sorry (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2)
+    ATTRIBUTE_NORETURN;
 extern void inform (location_t, const char *, ...) ATTRIBUTE_GCC_DIAG(2,3);
 extern void inform (rich_location *, const char *, ...) ATTRIBUTE_GCC_DIAG(2,3);
 extern void inform_n (location_t, unsigned HOST_WIDE_INT, const char *,
Index: gcc/diagnostic.c
===================================================================
--- gcc/diagnostic.c	2018-03-01 08:20:43.589534337 +0000
+++ gcc/diagnostic.c	2018-03-21 08:31:34.635677798 +0000
@@ -1407,6 +1407,20 @@  sorry (const char *gmsgid, ...)
   va_end (ap);
 }
 
+/* Same, but stop compilation immediately.  */
+
+void
+fatal_sorry (const char *gmsgid, ...)
+{
+  va_list ap;
+  va_start (ap, gmsgid);
+  rich_location richloc (line_table, input_location);
+  diagnostic_impl (&richloc, -1, gmsgid, &ap, DK_SORRY);
+  va_end (ap);
+
+  exit (FATAL_EXIT_CODE);
+}
+
 /* Return true if an error or a "sorry" has been seen.  Various
    processing is disabled after errors.  */
 bool
Index: gcc/calls.c
===================================================================
--- gcc/calls.c	2018-03-17 08:30:20.937100705 +0000
+++ gcc/calls.c	2018-03-21 08:31:34.635677798 +0000
@@ -4052,13 +4052,10 @@  expand_call (tree exp, rtx target, int i
 	      rtx_insn *before_arg = get_last_insn ();
 
 	      /* We don't allow passing huge (> 2^30 B) arguments
-	         by value.  It would cause an overflow later on.  */
+		 by value.  It would cause an overflow later on.  */
 	      if (constant_lower_bound (adjusted_args_size.constant)
 		  >= (1 << (HOST_BITS_PER_INT - 2)))
-	        {
-	          sorry ("passing too large argument on stack");
-		  continue;
-		}
+		fatal_sorry ("passing too large argument on stack");
 
 	      if (store_one_arg (&args[i], argblock, flags,
 				 adjusted_args_size.var != 0,
Index: gcc/testsuite/g++.dg/diagnostic/pr84964.C
===================================================================
--- /dev/null	2018-03-17 08:19:33.716019995 +0000
+++ gcc/testsuite/g++.dg/diagnostic/pr84964.C	2018-03-21 08:31:34.635677798 +0000
@@ -0,0 +1,6 @@ 
+/* { dg-do compile { target lp64 } } */
+
+struct a {
+  short b : 1ULL << 33; /* { dg-warning "width of 'a::b' exceeds its type" } */
+};
+void c(...) { c(a()); } /* { dg-message "sorry, unimplemented: passing too large argument on stack" } */