Re: [PATCH] Volatile bitfields vs. inline asm memory constraints

[Diego Novillo <dnovillo at google dot com>]

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.