On Thu, Aug 23, 2012 at 10:58 AM, Richard Guenther <richard.guent...@gmail.com> wrote: > On Thu, Aug 23, 2012 at 5:37 AM, Sandra Loosemore > <san...@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.