Re: [PATCH] fix wrong-code bug for -fstrict-volatile-bitfields

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Aug 23, 2012 at 10:58 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Aug 23, 2012 at 5:37 AM, Sandra Loosemore
> <sandra@codesourcery.com> wrote:
>> On 08/22/2012 03:27 PM, Eric Botcazou wrote:
>>>>
>>>> +             bool packedp = false;
>>>> +
>>>> +             if (TREE_CODE(to) == COMPONENT_REF
>>>> +&&  (TYPE_PACKED (TREE_TYPE (TREE_OPERAND (to, 0)))
>>>>
>>>> +                     || (TREE_CODE (TREE_OPERAND (to, 1)) == FIELD_DECL
>>>> +&&  DECL_PACKED (TREE_OPERAND (to, 1)))))
>>>>
>>>> +               packedp = true;
>>>>
>>>> note that this is not a reliable way of determining if a field is packed
>>>> (yes, the existing code is broken in the same way).  I realize that you
>>>> are only using this for diagnostic purposes, still something worth
>>>> mentioning.
>>>
>>>
>>> Indeed, ideally DECL_PACKED shouldn't be used outside stor-layout.c and
>>> dwarf2out.c in the middle-end.
>>>
>>>> I dislike passing around the packedp flag throughout the expander and to
>>>> warn inside the expander.  First, the flag has a name that can be
>>>> confusing
>>>> (it does not conservatively flag all packed accesses).  Second, why don't
>>>> you warn for this from inside the frontend when the bitfield access is
>>>> generated?  This way the diagnostic won't depend on optimization levels
>>>> and you avoid uglifying the expander.
>>>
>>>
>>> Seconded.  If the -fstrict-volatile-bitfields support is still incomplete,
>>> let's take this opportunity to clean it up.  Testing DECL_PACKED or
>>> issuing
>>> a warning from the RTL expander is to be avoided.
>>
>>
>> Well, I'm willing to give it a shot, but from a pragmatic point of view I've
>> only got about a week left to wind up the current batch of patches I've got
>> pending before I'm reassigned to a new project.  Can one of you give a more
>> specific outline of how this should be fixed, to increase the chances that I
>> can actually implement an acceptable solution in the time available?  In
>> particular, the packedp flag that's being passed down is *not* just used for
>> diagnostics; it changes code generation, and this code is scary enough that
>> I'm worried about preserving the current behavior (which IIUC is required by
>> the ARM EABI) if this is restructured.  If only the diagnostics should be
>> moved elsewhere, where, exactly?  Or is there just a better test that can be
>> used in the existing places to determine whether a field is packed?
>
> First of all the warning should be probably issued from stor-layout.c
> itself - see
> other cases where we warn about packed structs.  Yes, that means you'll
> get the warning even when there is no access but you'll only get it a
> single time.
>
> As of detecting whether a field is not aligned according to its mode, well,
> that's impossible if you just look at the field or its containing type (and thus
> for a warning without false positives / negatives from stor-layout).  It's also
> impossible to determine optimally (thus, it will say "not aligned according to
> its mode" in more cases).  The way you detect such misalignment is,
> given an access to such field,
>
>   get_object_alignment (exp) < GET_MODE_ALIGNMENT (DECL_MODE (field))
>
> when exp is a COMPONENT_REF with TREE_OPERAND (exp, 1) == field
> and field has non-BLK mode.
>
> Note that an access a.f with f being a volatile field may be represented
> by a volatile MEM_REF[&a, 16] where 16 is the byte offset relative to a.
>
> Now, if you are only interested in bitfields (note, bitfields which fit a mode
> are _not_ bitfields as far as optimization passes are concerned and thus
> may end up like above), then you probably want to look at
> DECL_BIT_FIELD_REPRESENTATIVE of the FIELD_DECL of such field.
> DECL_BIT_FIELD_REPRESENTATIVE constrains the offset / size said
> field is to be accessed and you should call get_object_alignment with
> a COMPONENT_REF using that field instead.  You also want to make
> sure DECL_BIT_FIELD_REPRESENTATIVE is computed correctly for
> volatile bitfields on ARM (stor-layout computes that).

In fact, you should probably implement code-generation constraints from
within the frontends by, for strict volatile bitfields, emitting loads/stores
using DECL_BIT_FIELD_REPRESENTATIVE (doing read-modify-write
explicitely).  Or maybe you can elaborate on the code-generation effects
of strict-volatile bitfields?

Richard.