On Wed, Jun 12, 2013 at 11:59 PM, Sandra Loosemore
<san...@codesourcery.com> wrote:
> Background: on ARM and some other targets, the ABI requires that volatile
> bit-fields be accessed atomically in a mode that corresponds to the declared
> type of the field, which conflicts with GCC's normal behavior of doing
> accesses in a mode that might correspond to the size of a general-purpose
> register, the size of the bit-field, or the bit range corresponding to the
> C++ memory model. This is what the -fstrict-volatile-bitfields flag does,
> and it is the default on ARM and other targets where the ABI requires this
> behavior.
>
> Both the original patch that added -fstrict-volatile-bitfields support and a
> subsequent followup patch that tried to unbreak handling of packed
> structures (where fields might not be properly aligned to do the single
> access otherwise mandated by -fstrict-volatile-bitfields) only handled
> bit-field reads, and not writes. Last year I submitted a patch we've had
> locally for some time to extend the existing implementation to writes, but
> it was rejected on the grounds that the current implementation is too broken
> to fix or extend. I didn't have time then to start over from scratch, and
> meanwhile, the bug reports about -fstrict-volatile-bitfields have continued
> to pile up. So let's try again to fix this, this time working from the
> ground up.
>
> From last year's discussion, it seemed that there were two primary
> objections to the current implementation:
>
> (1) It was seen as inappropriate that warnings about conflicts between
> unaligned fields and -fstrict-volatile-bitfields were being emitted during
> expand. It was suggested that any diagnostics ought to be emitted by the
> various language front ends instead.
>
> (2) The way packed fields are being detected is buggy and an abstraction
> violation, and passing around a packedp flag to all the bit-field expand
> functions is ugly.
>
> And, my own complaints about the current implementation:
>
> (3) Users expect packed structures to work even on targets where
> -fstrict-volatile-bitfields is the default, so the compiler shouldn't
> generate code for accesses to unaligned fields that either faults at run
> time due to the unaligned access or silently produces an incorrect result
> (e.g., by only accessing part of the bit-field), with or without a warning
> at compile time.
>
> (4) There's pointless divergence between the bit-field store and extract
> code that has led to a number of bugs.
>
> I've come up with a new patch that tries to address all these issues.
>
> For problem (1), I've eliminated the warnings from expand. I'm not opposed
> to adding them back to the front ends, as previously suggested, but given
> that they were only previously implemented for reads and not writes and that
> it was getting the different-warning-for-packed-fields behavior wrong in
> some cases, getting rid of the warnings is at least as correct as adding
> them for bit-field writes, too. ;-)
>
> I've killed the packedp flag from item (2) completely too.
>
> For problem (3), my reading of the ARM ABI document is that the requirements
> for atomic access to volatile bit-fields only apply to bit-fields that are
> aligned according to the ABI requirements. If a user has used GCC
> extensions to create a non-ABI-compliant packed structure where an atomic
> bit-field access of the correct size is not possible or valid on the target,
> then GCC ought to define some reasonable access behavior for those
> bit-fields too as a further extension -- whether or not it complies with the
> ABI requirements for unpacked structures -- rather than just generating
> invalid code. In particular, generating the access using whatever
> technique it would fall back to if -fstrict-volatile-bitfields didn't apply,
> in cases where it *cannot* be applied, seems perfectly reasonable to me.
>
> To address problem (4), I've tried to make the code for handling
> -fstrict-volatile-bitfields similar in the read and write cases. I think
> there is probably more that can be done here in terms of refactoring some of
> the now-common code and changing the interfaces to be more consistent as
> well, but I think it would be more clear to separate changes that are just
> code cleanup from those that are intended to change behavior. I'm willing
> to work on code refactoring as a followup patch if the maintainers recommend
> or require that.
>
> I've regression tested the attached patch on arm-none-eabi as well as
> bootstrapping and regression testing on x86_64-linux-gnu. I also did some
> spot testing on mipsisa32r2-sde-elf. I verified that all the new test cases
> pass on these targets with this patch. Without the patch, I saw these
> failures:
>
> pr23623.c: ARM, x86_64, MIPS
> pr48784-1.c: ARM, x86_64, MIPS
> pr48784-2.c: none
> pr56341-1.c: ARM, MIPS
> pr56341-2.c: none
> pr56997-1.c: ARM
> pr56997-2.c: ARM, MIPS
> pr56997-3.c: ARM, MIPS
> volatile-bitfields-3.c: ARM, x86_64, MIPS
>
> Here are some comments on specific parts of the patch.
>
> The "meat" of the patch is rewriting the logic for
> -fstrict-volatile-bitfields in extract_fixed_bit_field, and making
> store_fixed_bit_field use parallel logic.
>
> The change to make extract_bit_field_1 not skip over the
> simple_mem_bitfield_p case was necessary to avoid introducing new failures
> of the pr56997-* test cases on x86_64. store_bit_field_1 already has
> parallel code, except for getting the field mode passed in explicitly as an
> argument.
>
> The change to store_bit_field_1 and extract_bit_field_1 to make them skip
> both cheap register alternatives if -fstrict-volatile-bitfields applies,
> instead of just the first one, is for the volatile-bitfields-3.c test case.
> As noted above, this example was previously failing on all three targets I
> tried, and I was specifically using x86_64 to debug this one.
>
> The hack to get_bit_range fixes a regression in the pr23623 test case that
> has been present since the bit range support was added, I think. There might
> be a better place to do this, but it seems clear to me that when
> -fstrict-volatile-bitfields applies it has to override the bit range that
> would be used for normal accesses. It seemed to me during my incremental
> testing and debugging that this change was independent of the others (e.g.,
> it fixed the regression even with no other changes).
>
> Anyway, what do you think of this patch? It does fix a whole lot of bugs,
> and seems like at least an incremental improvement over the current code. I
> realize this is probably going to take a couple iterations to get right. If
> there are additional cleanups or refactoring of the bit-field code required,
> in addition to the functional changes, can they be done as followup patches
> or do I need to work out the entire series of patches in advance?
I think we can split the patch up, so let me do piecewise approval of changes.
The changes that remove the packedp flag passing around and remove
the warning code are ok. The store_bit_field_1 change is ok, as is
the similar extract_bit_field_1 change (guard the other register copying path).
Please split those out and commit them separately (I suppose they are
strict progressions in testing).
That leaves a much smaller set of changes for which I have one comment
for now:
@@ -4508,6 +4508,16 @@ get_bit_range (unsigned HOST_WIDE_INT *b
gcc_assert (TREE_CODE (exp) == COMPONENT_REF);
+ /* If -fstrict-volatile-bitfields was given and this is a volatile
+ access, then we must ignore any DECL_BIT_FIELD_REPRESENTATIVE and
+ do the access in the mode of the field. */
+ if (TREE_THIS_VOLATILE (exp)
+ && flag_strict_volatile_bitfields > 0)
+ {
+ *bitstart = *bitend = 0;
+ return;
+ }
with the past reviews where I said the implementation was broken anyway
I was hinting at that DECL_BIT_FIELD_REPRESENTATIVE should simply
be chosen in a way to adhere to flag_strict_volatile_bitfields. I had the
impression that this alone should be enough to implement strict volatile
bitfields correctly and no code in the backend would need to check
for flag_strict_volatile_bitfields. I may of course be wrong here, as
-fstrict-volatile-bitfields may apply to cases where the bitfield is _not_
declared volatile but only the decl? Thus,
struct X {
int i : 3;
};
volatile struct X x;
Is x.i an access that needs to adhere to strict volatile bitfield accesses?
Note that IIRC whether you short-cut get_bit_range in the above way
you still get the access to use the mode of the FIELD_DECL.
If the above constitutes a strict volatile bitfield access then the very
correct implementation of strict volatile bitfield accesses is to
adjust the memory accesses the frontends generate (consider
LTO and a TU compiled with -fstrict-volatile-bitfields and another TU
compiled with -fno-strict-volatile-bitfields). Which it probably does
reasonably well already (setting TREE_THIS_VOLATILE on the
outermost bit-field COMPONENT_REF). If that is not preserved
then converting the accesses to BIT_FIELD_REFs is another
possibility. That said, the behavior of GIMPLE IL should not change
dependent on a compile flag.
Richard.
> -Sandra
>
