| Message ID | 871sgdzv2y.fsf@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix ICE after sorry for big stack arguments (PR 84964) | expand |
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 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).
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 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
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
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" } */
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.