* [PATCH] Fix PR56344
@ 2013-02-26 18:27 Marek Polacek
2013-02-26 23:17 ` Joseph S. Myers
0 siblings, 1 reply; 12+ messages in thread
From: Marek Polacek @ 2013-02-26 18:27 UTC (permalink / raw)
To: GCC Patches
This "fixes" PR56344 by prohibiting passing arguments by value
of size >= 2^30 bytes. Probably no sane programmer would want to
do that, but it's better to issue an error than to segfault.
This would be a good opportunity to use __builtin_expect, but
we don't use that much in the codebase...
Regtested/bootstrapped on x86_64-linux, ok for trunk?
Do we need a testcase for this (compiling it is quite slow)?
2013-02-26 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-26 18:50:54.864084545 +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))
+ {
+ error ("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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] Fix PR56344 2013-02-26 18:27 [PATCH] Fix PR56344 Marek Polacek @ 2013-02-26 23:17 ` Joseph S. Myers 2013-02-27 9:56 ` Marek Polacek 0 siblings, 1 reply; 12+ messages in thread From: Joseph S. Myers @ 2013-02-26 23:17 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches 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?) -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR56344 2013-02-26 23:17 ` Joseph S. Myers @ 2013-02-27 9:56 ` Marek Polacek 2013-02-27 11:33 ` Richard Biener 0 siblings, 1 reply; 12+ messages in thread From: Marek Polacek @ 2013-02-27 9:56 UTC (permalink / raw) To: Joseph S. Myers; +Cc: GCC Patches 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. --- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR56344 2013-02-27 9:56 ` Marek Polacek @ 2013-02-27 11:33 ` Richard Biener 2013-02-27 17:38 ` Joseph S. Myers 0 siblings, 1 reply; 12+ messages in thread From: Richard Biener @ 2013-02-27 11:33 UTC (permalink / raw) To: Marek Polacek; +Cc: Joseph S. Myers, GCC Patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR56344 2013-02-27 11:33 ` Richard Biener @ 2013-02-27 17:38 ` Joseph S. Myers 2013-03-01 8:41 ` Richard Biener 0 siblings, 1 reply; 12+ messages in thread From: Joseph S. Myers @ 2013-02-27 17:38 UTC (permalink / raw) To: Richard Biener; +Cc: Marek Polacek, GCC Patches 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? -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR56344 2013-02-27 17:38 ` Joseph S. Myers @ 2013-03-01 8:41 ` Richard Biener 2013-03-05 16:06 ` Marek Polacek 0 siblings, 1 reply; 12+ messages in thread From: Richard Biener @ 2013-03-01 8:41 UTC (permalink / raw) To: Joseph S. Myers; +Cc: Marek Polacek, GCC Patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR56344 2013-03-01 8:41 ` Richard Biener @ 2013-03-05 16:06 ` Marek Polacek 2013-03-13 12:57 ` Marek Polacek 0 siblings, 1 reply; 12+ messages in thread From: Marek Polacek @ 2013-03-05 16:06 UTC (permalink / raw) To: Richard Biener; +Cc: Joseph S. Myers, GCC Patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR56344 2013-03-05 16:06 ` Marek Polacek @ 2013-03-13 12:57 ` Marek Polacek 2013-12-02 15:01 ` Richard Biener 0 siblings, 1 reply; 12+ messages in thread From: Marek Polacek @ 2013-03-13 12:57 UTC (permalink / raw) To: Richard Biener; +Cc: Joseph S. Myers, GCC Patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR56344 2013-03-13 12:57 ` Marek Polacek @ 2013-12-02 15:01 ` Richard Biener 2013-12-02 16:40 ` Marek Polacek 0 siblings, 1 reply; 12+ messages in thread From: Richard Biener @ 2013-12-02 15:01 UTC (permalink / raw) To: Marek Polacek; +Cc: Joseph S. Myers, GCC Patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR56344 2013-12-02 15:01 ` Richard Biener @ 2013-12-02 16:40 ` Marek Polacek 2013-12-02 19:19 ` Marek Polacek 0 siblings, 1 reply; 12+ messages in thread From: Marek Polacek @ 2013-12-02 16:40 UTC (permalink / raw) To: Richard Biener; +Cc: Joseph S. Myers, GCC Patches On Mon, Dec 02, 2013 at 04:01:05PM +0100, Richard Biener wrote: > 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. Just to be sure, did you mean to ok this patch (that is, the one with HOST_BITS_PER_INT)? Bootstrap/regtest in progress. 2013-12-02 Marek Polacek <polacek@redhat.com> PR middle-end/56344 * calls.c (expand_call): Disallow passing huge arguments by value. --- gcc/calls.c.mp4 2013-12-02 17:12:18.621057873 +0100 +++ gcc/calls.c 2013-12-02 17:32:35.523684716 +0100 @@ -3047,6 +3047,15 @@ 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 << (HOST_BITS_PER_INT - 1))) + { + 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR56344 2013-12-02 16:40 ` Marek Polacek @ 2013-12-02 19:19 ` Marek Polacek 2013-12-02 20:35 ` Richard Biener 0 siblings, 1 reply; 12+ messages in thread From: Marek Polacek @ 2013-12-02 19:19 UTC (permalink / raw) To: Richard Biener; +Cc: Joseph S. Myers, GCC Patches On Mon, Dec 02, 2013 at 05:40:33PM +0100, Marek Polacek wrote: > On Mon, Dec 02, 2013 at 04:01:05PM +0100, Richard Biener wrote: > > 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. Just to be sure, did you mean to ok this patch (that is, > the one with HOST_BITS_PER_INT)? > > Bootstrap/regtest in progress. > > 2013-12-02 Marek Polacek <polacek@redhat.com> > > PR middle-end/56344 > * calls.c (expand_call): Disallow passing huge arguments > by value. > > --- gcc/calls.c.mp4 2013-12-02 17:12:18.621057873 +0100 > +++ gcc/calls.c 2013-12-02 17:32:35.523684716 +0100 > @@ -3047,6 +3047,15 @@ 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 << (HOST_BITS_PER_INT - 1))) Surely I meant to use "HOST_BITS_PER_INT - 2" here. Marek ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR56344 2013-12-02 19:19 ` Marek Polacek @ 2013-12-02 20:35 ` Richard Biener 0 siblings, 0 replies; 12+ messages in thread From: Richard Biener @ 2013-12-02 20:35 UTC (permalink / raw) To: Marek Polacek; +Cc: Joseph S. Myers, GCC Patches Marek Polacek <polacek@redhat.com> wrote: >On Mon, Dec 02, 2013 at 05:40:33PM +0100, Marek Polacek wrote: >> On Mon, Dec 02, 2013 at 04:01:05PM +0100, Richard Biener wrote: >> > 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. Just to be sure, did you mean to ok this patch (that is, >> the one with HOST_BITS_PER_INT)? Yes, thanks, Richard. >> Bootstrap/regtest in progress. >> >> 2013-12-02 Marek Polacek <polacek@redhat.com> >> >> PR middle-end/56344 >> * calls.c (expand_call): Disallow passing huge arguments >> by value. >> >> --- gcc/calls.c.mp4 2013-12-02 17:12:18.621057873 +0100 >> +++ gcc/calls.c 2013-12-02 17:32:35.523684716 +0100 >> @@ -3047,6 +3047,15 @@ 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 << (HOST_BITS_PER_INT - 1))) > >Surely I meant to use "HOST_BITS_PER_INT - 2" here. > > Marek ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-12-02 20:35 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-02-26 18:27 [PATCH] Fix PR56344 Marek Polacek 2013-02-26 23:17 ` Joseph S. Myers 2013-02-27 9:56 ` Marek Polacek 2013-02-27 11:33 ` Richard Biener 2013-02-27 17:38 ` Joseph S. Myers 2013-03-01 8:41 ` Richard Biener 2013-03-05 16:06 ` Marek Polacek 2013-03-13 12:57 ` Marek Polacek 2013-12-02 15:01 ` Richard Biener 2013-12-02 16:40 ` Marek Polacek 2013-12-02 19:19 ` Marek Polacek 2013-12-02 20:35 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).