| Message ID | 20101122132854.0aca431a@rex.config |
|---|---|
| State | New |
| Headers | show |
On Mon, 22 Nov 2010 13:28:54 +0000 Julian Brown <julian@codesourcery.com> wrote: > Hi, > > This patch fixes the issue in the (Launchpad, not GCC) bug tracker: > > https://bugs.launchpad.net/gcc-linaro/+bug/675347 > > The problem was introduced by the patch from DJ to honour volatile > bitfield types: > > http://gcc.gnu.org/ml/gcc-patches/2010-06/msg01167.html Ping?
The key thing to check is if you have this field: int a:8; and it's 8-bit aligned anyway, that you still do an int-sized and int-aligned access.
On Tue, 30 Nov 2010 16:43:47 -0500 DJ Delorie <dj@redhat.com> wrote: > The key thing to check is if you have this field: > > int a:8; > > and it's 8-bit aligned anyway, that you still do an int-sized and > int-aligned access. I think this is fine: I checked the attached program with -fstrict-volatile-bitfields, both with and without my patch, and there's no change in generated assembly (which looks like it's doing the right thing, using container-sized accesses in each case, with read-modify-write where necessary). Cheers, Julian
On Wed, 1 Dec 2010 16:51:52 +0000 Julian Brown <julian@codesourcery.com> wrote: > On Tue, 30 Nov 2010 16:43:47 -0500 > DJ Delorie <dj@redhat.com> wrote: > > > The key thing to check is if you have this field: > > > > int a:8; > > > > and it's 8-bit aligned anyway, that you still do an int-sized and > > int-aligned access. > > I think this is fine: I checked the attached program with > -fstrict-volatile-bitfields, both with and without my patch, and > there's no change in generated assembly (which looks like it's doing > the right thing, using container-sized accesses in each case, with > read-modify-write where necessary). Ping^2? Thanks, Julian
On Wed, 8 Dec 2010 11:10:09 +0000 Julian Brown <julian@codesourcery.com> wrote: > On Wed, 1 Dec 2010 16:51:52 +0000 > Julian Brown <julian@codesourcery.com> wrote: > > > On Tue, 30 Nov 2010 16:43:47 -0500 > > DJ Delorie <dj@redhat.com> wrote: > > > > > The key thing to check is if you have this field: > > > > > > int a:8; > > > > > > and it's 8-bit aligned anyway, that you still do an int-sized and > > > int-aligned access. > > > > I think this is fine: I checked the attached program with > > -fstrict-volatile-bitfields, both with and without my patch, and > > there's no change in generated assembly (which looks like it's doing > > the right thing, using container-sized accesses in each case, with > > read-modify-write where necessary). > > Ping^2? Ping^3? Cheers, Julian
I hope you're not waiting for *my* approval, it's not part of the code I can do that for...
On Wed, 5 Jan 2011 15:43:19 -0500 DJ Delorie <dj@redhat.com> wrote: > I hope you're not waiting for *my* approval, it's not part of the code > I can do that for... I guess I'm waiting for a global reviewer, since I suppose expr.c doesn't fall under any of the other umbrella maintainership areas (except perhaps "RTL optimizers"? CC'ing Eric just in case :-)). Cheers, Julian (who doesn't post many non-target-specific patches...)
> I guess I'm waiting for a global reviewer, since I suppose expr.c > doesn't fall under any of the other umbrella maintainership areas > (except perhaps "RTL optimizers"? CC'ing Eric just in case :-)). This is formally outside of my purview though.
On 05/01/2011 21:17, Julian Brown wrote: > On Wed, 5 Jan 2011 15:43:19 -0500 > DJ Delorie <dj@redhat.com> wrote: > >> I hope you're not waiting for *my* approval, it's not part of the code >> I can do that for... > > I guess I'm waiting for a global reviewer, since I suppose expr.c > doesn't fall under any of the other umbrella maintainership areas > (except perhaps "RTL optimizers"? I would have thought "middle-end". cheers, DaveK
On Wed, 05 Jan 2011 22:06:00 +0000 Dave Korn <dave.korn.cygwin@gmail.com> wrote: > On 05/01/2011 21:17, Julian Brown wrote: > > On Wed, 5 Jan 2011 15:43:19 -0500 > > DJ Delorie <dj@redhat.com> wrote: > > > >> I hope you're not waiting for *my* approval, it's not part of the > >> code I can do that for... > > > > I guess I'm waiting for a global reviewer, since I suppose expr.c > > doesn't fall under any of the other umbrella maintainership areas > > (except perhaps "RTL optimizers"? > > I would have thought "middle-end". OK, thanks. I assume the correct etiquette is *not* to CC the whole list of middle-end reviewers in these cases... I guess at least one of them will be reading the list anyway. Subject tweaked :-). Cheers, Julian
On Wed, 5 Jan 2011 22:40:31 +0000 Julian Brown <julian@codesourcery.com> wrote: > On Wed, 05 Jan 2011 22:06:00 +0000 > Dave Korn <dave.korn.cygwin@gmail.com> wrote: > > > On 05/01/2011 21:17, Julian Brown wrote: > > > On Wed, 5 Jan 2011 15:43:19 -0500 > > > DJ Delorie <dj@redhat.com> wrote: > > > > > >> I hope you're not waiting for *my* approval, it's not part of the > > >> code I can do that for... > > > > > > I guess I'm waiting for a global reviewer, since I suppose expr.c > > > doesn't fall under any of the other umbrella maintainership areas > > > (except perhaps "RTL optimizers"? > > > > I would have thought "middle-end". > > OK, thanks. I assume the correct etiquette is *not* to CC the > whole list of middle-end reviewers in these cases... I guess at least > one of them will be reading the list anyway. Subject tweaked :-). Well that didn't seem to work :-). Middle-end maintainers, could one of you take a look at this patch please? Quick context: the bug this fixes causes the QT library (and QT-dependent apps) to fail to build on ARM. Also CC'ing Mark, who reviewed DJ Delorie's patch initially. Thanks, Julian
On Wed, 12 Jan 2011 18:11:14 +0000 Julian Brown <julian@codesourcery.com> wrote: > On Wed, 5 Jan 2011 22:40:31 +0000 > Julian Brown <julian@codesourcery.com> wrote: > > > On Wed, 05 Jan 2011 22:06:00 +0000 > > Dave Korn <dave.korn.cygwin@gmail.com> wrote: > > > > I guess I'm waiting for a global reviewer, since I suppose > > > > expr.c doesn't fall under any of the other umbrella > > > > maintainership areas (except perhaps "RTL optimizers"? > > > > > > I would have thought "middle-end". Ping? Julian
On Mon, Nov 22, 2010 at 08:28, Julian Brown <julian@codesourcery.com> wrote: > Hi, > > This patch fixes the issue in the (Launchpad, not GCC) bug tracker: > > https://bugs.launchpad.net/gcc-linaro/+bug/675347 > > The problem was introduced by the patch from DJ to honour volatile > bitfield types: > > http://gcc.gnu.org/ml/gcc-patches/2010-06/msg01167.html > > but not exposed (on ARM) until the option was made the default (on the > Linaro branch) -- it's not yet the default on mainline. > > The issue is as follows: after DJ's patch and with > -fstrict-volatile-bitfields, in expr.c:expand_expr_real_1, the if > condition with the comment "In cases where an aligned union has an > unaligned object as a field, we might be extracting a BLKmode value > from an integer-mode (e.g., SImode) object [...]" triggers for a normal > (non-bitfield) volatile field of a struct/class. > > But, this appears to be over-eager: in the particular case mentioned > above, when expanding a "volatile int" struct field used as a memory > constraint for an inline asm, we end up with something which is no > longer addressable (I think because of the actions of > extract_bit_field). So, compilation aborts. > > My proposed fix is to restrict the conditional by only making it execute > for -fstrict-volatile-bitfields only for non-naturally-aligned accesses: > this appears to work (fixes test in question, and no regressions for > cross to ARM Linux, gcc/g++/libstdc++, with -fstrict-volatile-bitfields > turned on), but I don't know if there will be unintended consequences. > DJ, does it look sane to you? > > Incidentally the constraints in the inline asm in the Launchpad > testcase might be slightly dubious (attempting to force (mem (reg)) by > using both "+m" (var) and "r" (&var) constraints), but replacing > them with e.g.: > > asm volatile("0:\n" > "ldrex %[newValue], %[_q_value]\n" > "sub %[newValue], %[newValue], #1\n" > "strex %[result], %[newValue], %[_q_value]\n" > "teq %[result], #0\n" > "bne 0b\n" > : [newValue] "=&r" (newValue), > [result] "=&r" (result) > : [_q_value] "Q" (_q_value) > : "cc", "memory"); > > still leads to a warning (not an error) with trunk and > -fstrict-volatile-bitfields: > > atomic-changed.cc:24:35: warning: use of memory input without lvalue in > asm operand 2 is deprecated [enabled by default] > > The warning goes away with the attached patch. So, I don't think the > problem is purely that the original inline asm is invalid. > > OK to apply, or any comments? Could you add a comment before the test to describe why you are excluding non-natural alignments? OK with that change, though I think you'll have to stage this for 4.7. Diego.
Index: gcc/expr.c =================================================================== --- gcc/expr.c (revision 166945) +++ gcc/expr.c (working copy) @@ -9124,7 +9124,8 @@ expand_expr_real_1 (tree exp, rtx target && modifier != EXPAND_INITIALIZER) /* If the field is volatile, we always want an aligned access. */ - || (volatilep && flag_strict_volatile_bitfields > 0) + || (volatilep && flag_strict_volatile_bitfields > 0 + && (bitpos % GET_MODE_ALIGNMENT (mode) != 0)) /* If the field isn't aligned enough to fetch as a memref, fetch it as a bit field. */ || (mode1 != BLKmode
Hi, This patch fixes the issue in the (Launchpad, not GCC) bug tracker: https://bugs.launchpad.net/gcc-linaro/+bug/675347 The problem was introduced by the patch from DJ to honour volatile bitfield types: http://gcc.gnu.org/ml/gcc-patches/2010-06/msg01167.html but not exposed (on ARM) until the option was made the default (on the Linaro branch) -- it's not yet the default on mainline. The issue is as follows: after DJ's patch and with -fstrict-volatile-bitfields, in expr.c:expand_expr_real_1, the if condition with the comment "In cases where an aligned union has an unaligned object as a field, we might be extracting a BLKmode value from an integer-mode (e.g., SImode) object [...]" triggers for a normal (non-bitfield) volatile field of a struct/class. But, this appears to be over-eager: in the particular case mentioned above, when expanding a "volatile int" struct field used as a memory constraint for an inline asm, we end up with something which is no longer addressable (I think because of the actions of extract_bit_field). So, compilation aborts. My proposed fix is to restrict the conditional by only making it execute for -fstrict-volatile-bitfields only for non-naturally-aligned accesses: this appears to work (fixes test in question, and no regressions for cross to ARM Linux, gcc/g++/libstdc++, with -fstrict-volatile-bitfields turned on), but I don't know if there will be unintended consequences. DJ, does it look sane to you? Incidentally the constraints in the inline asm in the Launchpad testcase might be slightly dubious (attempting to force (mem (reg)) by using both "+m" (var) and "r" (&var) constraints), but replacing them with e.g.: asm volatile("0:\n" "ldrex %[newValue], %[_q_value]\n" "sub %[newValue], %[newValue], #1\n" "strex %[result], %[newValue], %[_q_value]\n" "teq %[result], #0\n" "bne 0b\n" : [newValue] "=&r" (newValue), [result] "=&r" (result) : [_q_value] "Q" (_q_value) : "cc", "memory"); still leads to a warning (not an error) with trunk and -fstrict-volatile-bitfields: atomic-changed.cc:24:35: warning: use of memory input without lvalue in asm operand 2 is deprecated [enabled by default] The warning goes away with the attached patch. So, I don't think the problem is purely that the original inline asm is invalid. OK to apply, or any comments? Julian ChangeLog gcc/ * expr.c (expand_expr_real_1): Only use BLKmode for volatile accesses which are not naturally aligned.