| Message ID | CAFiYyc0PgY3miKhjRYAvkWPiCxFc7AOcrN9G=mSL2rS0Qswy1w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 3 Dec 2013 15:12:05, Richard Biener wrote: > > On Tue, Dec 3, 2013 at 2:10 PM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Tue, Dec 3, 2013 at 1:48 PM, Bernd Edlinger >> <bernd.edlinger@hotmail.de> wrote: >>> Hi Jeff, >>> >>> please find attached the patch (incl. test cases) for the unaligned read BUG that I found while investigating >>> on PR#57748: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57748 >>> >>> one test case is this one: >>> >>> pr57748-3.c: >>> /* PR middle-end/57748 */ >>> /* { dg-do run } */ >>> /* wrong code in expand_expr_real_1. */ >>> >>> #include <stdlib.h> >>> >>> extern void abort (void); >>> >>> typedef long long V >>> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); >>> >>> typedef struct S { V a; V b[0]; } P __attribute__((aligned (1))); >>> >>> struct __attribute__((packed)) T { char c; P s; }; >>> >>> void __attribute__((noinline, noclone)) >>> check (P *p) >>> { >>> if (p->b[0][0] != 3 || p->b[0][1] != 4) >>> abort (); >>> } >>> >>> void __attribute__((noinline, noclone)) >>> foo (struct T *t) >>> { >>> V a = { 3, 4 }; >>> t->s.b[0] = a; >>> } >>> >>> int >>> main () >>> { >>> struct T *t = (struct T *) calloc (128, 1); >>> >>> foo (t); >>> check (&t->s); >>> >>> free (t); >>> return 0; >>> } >>> >>> >>> and the other one is >>> pr57748-4.c: >>> /* PR middle-end/57748 */ >>> /* { dg-do run } */ >>> /* wrong code in expand_expr_real_1. */ >>> >>> #include <stdlib.h> >>> >>> extern void abort (void); >>> >>> typedef long long V >>> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); >>> >>> typedef struct S { V b[1]; } P __attribute__((aligned (1))); >>> >>> struct __attribute__((packed)) T { char c; P s; }; >>> >>> void __attribute__((noinline, noclone)) >>> check (P *p) >>> { >>> if (p->b[1][0] != 3 || p->b[1][1] != 4) >>> abort (); >>> } >>> >>> void __attribute__((noinline, noclone)) >>> foo (struct T *t) >>> { >>> V a = { 3, 4 }; >>> t->s.b[1] = a; >>> } >>> >>> int >>> main () >>> { >>> struct T *t = (struct T *) calloc (128, 1); >>> >>> foo (t); >>> check (&t->s); >>> >>> free (t); >>> return 0; >>> } >>> >>> >>> The patch does add a boolean "expand_reference" parameter to expand_expr_real and >>> expand_expr_real_1. I pass true when I intend to use the returned memory context >>> as an array reference, instead of a value. At places where mis-aligned values are extracted, >>> I do not return a register with the extracted mis-aligned value if expand_reference is true. >>> When I have a VIEW_CONVERT_EXPR I pay attention to pass down the outer "expand_reference" >>> to the inner expand_expr_real call. Expand_reference, is pretty much similar to the >>> expand_modifier "EXPAND_MEMORY". >>> >>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu (many times). >>> >>> Ok for trunk? >> >> It still feels like papering over the underlying issue. Let me have a >> second (or third?) look. > > Few comments on your patch. > > @@ -9520,6 +9526,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac > align = get_object_alignment (exp); > if (modifier != EXPAND_WRITE > && modifier != EXPAND_MEMORY > + && !expand_reference > && mode != BLKmode > && align < GET_MODE_ALIGNMENT (mode) > /* If the target does not have special handling for unaligned > > (TARGET_MEM_REF), expand_reference should never be true here, > there may be no component-refs around TARGET_MEM_REFs. > Ok, I was not sure. Removed - Thanks. > You miss adjusting the VIEW_CONVERT_EXPR path? (line-numbers > are off a lot in your patch, context doesn't help very much :/ Does not > seem to be against 4.8 either ...) > Sorry, The line-numbers moved a lot> 100 lines. The patch is against 4.9-trunk. Re-generated. > Index: gcc/cfgexpand.c > =================================================================== > --- gcc/cfgexpand.c (revision 204411) > +++ gcc/cfgexpand.c (working copy) > @@ -2189,7 +2189,7 @@ expand_call_stmt (gimple stmt) > if (lhs) > expand_assignment (lhs, exp, false); > else > - expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL); > + expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL, false); > > mark_transaction_restart_calls (stmt); > } > > this should use > > expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); > > anyway. > expand_expr_real_1 is expand_expr_real without the ERROR check? Ok, changed to expand_expr. > @@ -10286,7 +10297,10 @@ expand_expr_real_1 (tree exp, rtx target, enum mac > op0 = copy_rtx (op0); > set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type))); > } > - else if (mode != BLKmode > + else if (modifier != EXPAND_WRITE > + && modifier != EXPAND_MEMORY > + && !expand_reference > + && mode != BLKmode > && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode) > /* If the target does have special handling for unaligned > loads of mode then use them. */ > > @@ -10307,6 +10321,9 @@ expand_expr_real_1 (tree exp, rtx target, enum mac > return reg; > } > else if (STRICT_ALIGNMENT > + && modifier != EXPAND_WRITE > + && modifier != EXPAND_MEMORY > + && !expand_reference > && mode != BLKmode > && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode)) > { > > why the unrelated change to add the modifier checks? Looks like both > if cases are close by and factoring out common code like > > else if (!expand_reference > && mode != BLKmode > && MEM_ALIGN (... > { > if (...) > else if (STRICT_ALIGNMENT) > > would be better, also matching the other similar occurances. > Ok, re-factored that if cascade. Thanks. Bernd. > Looking for some more time your patch may be indeed the easiest > without big re-factoring. > > Richard. > >> Richard. >> >>> >>> Thanks >>> Bernd. 2013-11-07 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * expr.h (expand_expr_real, expand_expr_real_1): Add new parameter expand_reference. (expand_expr, expand_normal): Adjust. * expr.c (expand_expr_real, expand_expr_real_1): Add new parameter expand_reference. Use expand_reference to expand inner references. (store_expr): Adjust. * cfgexpand.c (expand_call_stmt): Adjust. testsuite: 2013-11-07 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * gcc.dg/torture/pr57748-3.c: New test. * gcc.dg/torture/pr57748-4.c: New test.
On 12/04/13 01:16, Bernd Edlinger wrote: > >> Looking for some more time your patch may be indeed the easiest >> without big re-factoring. Richard (or Bernd), can you comment on why? Something seems "off" here. Why do we need to handle inner references here specially? If feels like we're catering to broken code elsewhere in GCC. As for the patch itself. In a few places within expand_expr_real_1 you changed calls to expand_expr to instead call expand_expr_real. ISTM you could have gotten the same effect by just adding your extra argument to the existing code? Jeff
Hi, On Thu, 5 Dec 2013 22:16:15, Jeff Law wrote: > > On 12/04/13 01:16, Bernd Edlinger wrote: > >> >>> Looking for some more time your patch may be indeed the easiest >>> without big re-factoring. > Richard (or Bernd), can you comment on why? Something seems "off" here. > > Why do we need to handle inner references here specially? If feels > like we're catering to broken code elsewhere in GCC. > My first attempt at fixing this was just a one-line change. like: @@ -9905,7 +9861,7 @@ expand_expr_real_1 (tree exp, rtx target && modifier != EXPAND_STACK_PARM ? target : NULL_RTX), VOIDmode, - modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); + EXPAND_MEMORY); But the problem with that was, that it might loose too much information... i.e. little we know what tem might be, and what a change to the original expand_modifier would have done at other places where we had no problems at all. So this current version of the patch is a trying to get as much flexibility as possible while changing as little as possible at the same time. Technically we only have to switch off some places in MEM_REF, and VIEW_CONVERT_EXPR, where an unaligned value is placed in a register, which turns out to be unnecessary in this special context. > As for the patch itself. In a few places within expand_expr_real_1 you > changed calls to expand_expr to instead call expand_expr_real. ISTM > you could have gotten the same effect by just adding your extra argument > to the existing code? > Yes, but one goal is to keep the patch-file as small as possible, and expand_expr is used everywhere. Actually expand_expr is just a wrapper for expand_expr_real (exp, target, mode, modifier, NULL, false) Therefore I replaced expand_expr with expand_expr_real at these few places, to keep the change smaller. Another point is, that I do not want to pass true for expand_reference at any other place than from exand_expr_real_1 where the inner reference is expanded. Thanks Bernd. > > Jeff
On Fri, Dec 6, 2013 at 6:16 AM, Jeff Law <law@redhat.com> wrote: > On 12/04/13 01:16, Bernd Edlinger wrote: > >> >>> Looking for some more time your patch may be indeed the easiest >>> without big re-factoring. > > Richard (or Bernd), can you comment on why? Something seems "off" here. > > Why do we need to handle inner references here specially? If feels like > we're catering to broken code elsewhere in GCC. The issue is that we handle expansion of misaligned moves in the code we recurse to - but that misaligned move handling can only work at the "outermost" level of the recursion as it is supposed to see the "real" access (alignment and mode of the memory access, not of some base of it). So we need a recursion that skips this part (and others - which already works), just processing as-if in "expand the base of some memory access" mode. That we recurse at all when expanding memory accesses makes this expansion path quite a twisted maze - which is why I suggested to re-factor the whole thing to not require recursion (but that will be a very big change, not suitable for this stage). The easiest is to add a flag to indicate the "we're-expanding-the-base", doing another expand modifier doesn't work as they are not combinable and the passed modifier is already modified for the recursion - and that dependent on stuff. "Fixing" the mode of the base object isn't really fixing the fact that the recursion shouldn't even try to generate a movmisalign mem, it just papers over this issue by making it (hopefully) never trigger (at least for valid code) for bases. Richard. > As for the patch itself. In a few places within expand_expr_real_1 you > changed calls to expand_expr to instead call expand_expr_real. ISTM you > could have gotten the same effect by just adding your extra argument to the > existing code? > > > Jeff
On 12/06/13 03:06, Richard Biener wrote: > > The issue is that we handle expansion of misaligned moves in the code > we recurse to - but that misaligned move handling can only work at > the "outermost" level of the recursion as it is supposed to see the > "real" access (alignment and mode of the memory access, not of > some base of it). > > So we need a recursion that skips this part (and others - which already > works), just processing as-if in "expand the base of some memory access" > mode. So it's really not a case of someone outside expand_expr needing different behaviour, but a case of stopping the recursion within expand_expr. That's a bit less concerning. > > That we recurse at all when expanding memory accesses makes this > expansion path quite a twisted maze - which is why I suggested to > re-factor the whole thing to not require recursion (but that will be > a very big change, not suitable for this stage). No doubt. In general expand_expr is a mess and has been, well, forever. > > The easiest is to add a flag to indicate the "we're-expanding-the-base", > doing another expand modifier doesn't work as they are not combinable > and the passed modifier is already modified for the recursion - and that > dependent on stuff. We could always make the modifiers a bitmask, but probably not something we really need to do during stage3. > > "Fixing" the mode of the base object isn't really fixing the fact that > the recursion shouldn't even try to generate a movmisalign mem, > it just papers over this issue by making it (hopefully) never trigger > (at least for valid code) for bases. Ok, that answers the other question I had after looking at other parts of this thread. Though one could argue that the modes are in fact wrong. Jeff
On 12/06/13 01:51, Bernd Edlinger wrote: >>> As for the patch itself. In a few places within expand_expr_real_1 you >> changed calls to expand_expr to instead call expand_expr_real. ISTM >> you could have gotten the same effect by just adding your extra argument >> to the existing code? >> > > Yes, but one goal is to keep the patch-file as small as possible, > and expand_expr is used everywhere. > > Actually expand_expr is just a wrapper for > > expand_expr_real (exp, target, mode, modifier, NULL, false) Sorry, I glossed over that -- mentally I saw the change to expand_expr and assumed you passed the new flag through. But that's not how the change is implemented. My bad. So clearly for the cases where you want the flag to be true, you can't go through the wrapper. Again, my bad. jeff
Index: gcc/cfgexpand.c =================================================================== --- gcc/cfgexpand.c (revision 204411) +++ gcc/cfgexpand.c (working copy) @@ -2189,7 +2189,7 @@ expand_call_stmt (gimple stmt) if (lhs) expand_assignment (lhs, exp, false); else - expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL); + expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL, false); mark_transaction_restart_calls (stmt); }