Patchwork Fix PR56344

login
register
mail settings
Submitter Marek Polacek
Date Feb. 27, 2013, 9:56 a.m.
Message ID <20130227095625.GA15445@redhat.com>
Download mbox | patch
Permalink /patch/223575/
State New
Headers show

Comments

Marek Polacek - Feb. 27, 2013, 9:56 a.m.
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?

2013-02-27  Marek Polacek  <polacek@redhat.com>

	PR middle-end/56344
	* calls.c (expand_call): Disallow passing huge arguments
	by value.


	Marek
Richard Guenther - Feb. 27, 2013, 11:33 a.m.
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
Joseph S. Myers - Feb. 27, 2013, 5:38 p.m.
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?
Richard Guenther - March 1, 2013, 8:41 a.m.
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
Marek Polacek - March 5, 2013, 4:06 p.m.
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
Marek Polacek - March 13, 2013, 12:57 p.m.
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
Richard Guenther - Dec. 2, 2013, 3:01 p.m.
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

Patch

--- 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)