| Message ID | 20130227095625.GA15445@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Feb 27, 2013 at 10:56 AM, Marek Polacek <polacek@redhat.com> wrote: > On Tue, Feb 26, 2013 at 11:17:22PM +0000, Joseph S. Myers wrote: >> On Tue, 26 Feb 2013, Marek Polacek wrote: >> >> > + /* We don't allow passing huge (> 2^30 B) arguments >> > + by value. It would cause an overflow later on. */ >> > + if (adjusted_args_size.constant >= (1 << 30)) >> > + { >> > + error ("passing too large argument on stack"); >> > + continue; >> >> This should be sorry () not error (), as a compiler limitation rather than >> a defect in the user's program. (And is input_location set to something >> useful here so the diagnostic points to the argument in question rather >> than e.g. to the end of the function containing the problem call?) > > Okay, changed back to sorry (). I'd think that input_location is fine > here, for e.g. > > struct S { unsigned char s[1 << 30]; } s; > extern void foo (struct S); > > void > bar (void) > { > foo (s); > } > > we get: > pr56344.c: In function ‘bar’: > pr56344.c:7:7: sorry, unimplemented: passing too large argument on stack > foo (s); > ^ > > Ok now? Wouldn't it be better to simply pass this using the variable size handling code? Thus, initialize args_size.var for too large constant size instead? Also args_size.constant is HOST_WIDE_INT, so I'm not sure where you derive the magic number 1<<30 from. I'd expect it to be sth like if (!host_integer_p (bit-size, 1)) init variable-size this has the advantage to move the error to runtime instead of compile-time which is better than rejecting valid C code that will work when it is not executed at runtime. Richard. > 2013-02-27 Marek Polacek <polacek@redhat.com> > > PR middle-end/56344 > * calls.c (expand_call): Disallow passing huge arguments > by value. > > --- gcc/calls.c.mp 2013-02-26 17:04:33.159555349 +0100 > +++ gcc/calls.c 2013-02-27 10:44:02.254461200 +0100 > @@ -3037,6 +3037,14 @@ expand_call (tree exp, rtx target, int i > { > rtx before_arg = get_last_insn (); > > + /* We don't allow passing huge (> 2^30 B) arguments > + by value. It would cause an overflow later on. */ > + if (adjusted_args_size.constant >= (1 << 30)) > + { > + sorry ("passing too large argument on stack"); > + continue; > + } > + > if (store_one_arg (&args[i], argblock, flags, > adjusted_args_size.var != 0, > reg_parm_stack_space) > > Marek
On Wed, 27 Feb 2013, Richard Biener wrote: > Wouldn't it be better to simply pass this using the variable size handling > code? Thus, initialize args_size.var for too large constant size instead? Would that be compatible with the ABI definition of how a large (constant size) argument should be passed?
On Wed, Feb 27, 2013 at 6:38 PM, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Wed, 27 Feb 2013, Richard Biener wrote: > >> Wouldn't it be better to simply pass this using the variable size handling >> code? Thus, initialize args_size.var for too large constant size instead? > > Would that be compatible with the ABI definition of how a large (constant > size) argument should be passed? I'm not sure. Another alternative is to expand to __builtin_trap (), but that's probably not easy at this very point. Or simply fix the size calculation to not overflow (either don't count bits or use a double-int). Richard. > -- > Joseph S. Myers > joseph@codesourcery.com
On Fri, Mar 01, 2013 at 09:41:27AM +0100, Richard Biener wrote: > On Wed, Feb 27, 2013 at 6:38 PM, Joseph S. Myers > <joseph@codesourcery.com> wrote: > > On Wed, 27 Feb 2013, Richard Biener wrote: > > > >> Wouldn't it be better to simply pass this using the variable size handling > >> code? Thus, initialize args_size.var for too large constant size instead? > > > > Would that be compatible with the ABI definition of how a large (constant > > size) argument should be passed? > > I'm not sure. Another alternative is to expand to __builtin_trap (), but that's > probably not easy at this very point. > > Or simply fix the size calculation to not overflow (either don't count bits > or use a double-int). I don't think double_int will help us here. We won't detect overflow, because we overflowed here (when lower_bound is an int): lower_bound = INTVAL (XEXP (XEXP (arg->stack_slot, 0), 1)); The value from INTVAL () fits when lower_bound is a double_int, but then: i = lower_bound; ... stack_usage_map[i] the size of stack_usage_map is stored in highest_outgoing_arg_in_use, which is an int, so we're limited by an int size here. Changing the type of highest_outgoing_arg_in_use from an int to a double_int isn't worth the trouble, IMHO. Maybe the original approach, only with sorry () instead of error () and e.g. HOST_BITS_PER_INT - 1 instead of 30 would be appropriate after all. Dunno. Marek
Ping. On Tue, Mar 05, 2013 at 05:06:21PM +0100, Marek Polacek wrote: > On Fri, Mar 01, 2013 at 09:41:27AM +0100, Richard Biener wrote: > > On Wed, Feb 27, 2013 at 6:38 PM, Joseph S. Myers > > <joseph@codesourcery.com> wrote: > > > On Wed, 27 Feb 2013, Richard Biener wrote: > > > > > >> Wouldn't it be better to simply pass this using the variable size handling > > >> code? Thus, initialize args_size.var for too large constant size instead? > > > > > > Would that be compatible with the ABI definition of how a large (constant > > > size) argument should be passed? > > > > I'm not sure. Another alternative is to expand to __builtin_trap (), but that's > > probably not easy at this very point. > > > > Or simply fix the size calculation to not overflow (either don't count bits > > or use a double-int). > > I don't think double_int will help us here. We won't detect overflow, > because we overflowed here (when lower_bound is an int): > lower_bound = INTVAL (XEXP (XEXP (arg->stack_slot, 0), 1)); > The value from INTVAL () fits when lower_bound is a double_int, but > then: > i = lower_bound; > ... > stack_usage_map[i] > the size of stack_usage_map is stored in highest_outgoing_arg_in_use, > which is an int, so we're limited by an int size here. > Changing the type of highest_outgoing_arg_in_use from an int to a > double_int isn't worth the trouble, IMHO. > > Maybe the original approach, only with sorry () instead of error () > and e.g. HOST_BITS_PER_INT - 1 instead of 30 would be appropriate > after all. Dunno. > > Marek
On Wed, Mar 13, 2013 at 1:57 PM, Marek Polacek <polacek@redhat.com> wrote: > Ping. Ok. (yay, oldest patch in my review queue ...) Thanks, Richard. > On Tue, Mar 05, 2013 at 05:06:21PM +0100, Marek Polacek wrote: >> On Fri, Mar 01, 2013 at 09:41:27AM +0100, Richard Biener wrote: >> > On Wed, Feb 27, 2013 at 6:38 PM, Joseph S. Myers >> > <joseph@codesourcery.com> wrote: >> > > On Wed, 27 Feb 2013, Richard Biener wrote: >> > > >> > >> Wouldn't it be better to simply pass this using the variable size handling >> > >> code? Thus, initialize args_size.var for too large constant size instead? >> > > >> > > Would that be compatible with the ABI definition of how a large (constant >> > > size) argument should be passed? >> > >> > I'm not sure. Another alternative is to expand to __builtin_trap (), but that's >> > probably not easy at this very point. >> > >> > Or simply fix the size calculation to not overflow (either don't count bits >> > or use a double-int). >> >> I don't think double_int will help us here. We won't detect overflow, >> because we overflowed here (when lower_bound is an int): >> lower_bound = INTVAL (XEXP (XEXP (arg->stack_slot, 0), 1)); >> The value from INTVAL () fits when lower_bound is a double_int, but >> then: >> i = lower_bound; >> ... >> stack_usage_map[i] >> the size of stack_usage_map is stored in highest_outgoing_arg_in_use, >> which is an int, so we're limited by an int size here. >> Changing the type of highest_outgoing_arg_in_use from an int to a >> double_int isn't worth the trouble, IMHO. >> >> Maybe the original approach, only with sorry () instead of error () >> and e.g. HOST_BITS_PER_INT - 1 instead of 30 would be appropriate >> after all. Dunno. >> >> Marek
--- gcc/calls.c.mp 2013-02-26 17:04:33.159555349 +0100 +++ gcc/calls.c 2013-02-27 10:44:02.254461200 +0100 @@ -3037,6 +3037,14 @@ expand_call (tree exp, rtx target, int i { rtx before_arg = get_last_insn (); + /* We don't allow passing huge (> 2^30 B) arguments + by value. It would cause an overflow later on. */ + if (adjusted_args_size.constant >= (1 << 30)) + { + sorry ("passing too large argument on stack"); + continue; + } + if (store_one_arg (&args[i], argblock, flags, adjusted_args_size.var != 0, reg_parm_stack_space)