On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
On Sun, 20 Oct 2013 20:23:49, Sandra Loosemore wrote:
I tried a backport to GCC 4.8 and tested on arm-none-eabi. On the new
pr56997-1.c testcase, it got stuck in infinite recursion between
store_split_bit_field/store_fixed_bit_field and/or
extract_split_bit_field/extract_fixed_bit_field. This didn't show up in
my previous mainline testing.
[snip]
I have attached an update to your patch, that should
a) fix the recursion problem.
b) restrict the -fstrict-volatile-bitfields to not violate the C++ memory model.
Thanks for picking up the ball on this -- I've been busy with other
tasks for several days.
I again tried backporting the patch series along with your fix to GCC
4.8 and tested on arm-none-eabi. I found that it was still getting
stuck in infinite recursion unless the test from this patch hunk
@@ -1699,6 +1711,14 @@ extract_bit_field (rtx str_rtx, unsigned
{
enum machine_mode mode1;
+ /* Handle extraction of unaligned fields,
+ this can happen in -fstrict-volatile-bitfields. */
+ if (GET_MODE_BITSIZE (GET_MODE (str_rtx)) != 0
+ && GET_MODE_BITSIZE (GET_MODE (str_rtx)) < GET_MODE_BITSIZE (word_mode)
+ && bitnum % GET_MODE_BITSIZE (GET_MODE (str_rtx)) + bitsize
+ > GET_MODE_BITSIZE (GET_MODE (str_rtx)) )
+ str_rtx = adjust_address (str_rtx, word_mode, 0);
+
/* Handle -fstrict-volatile-bitfields in the cases where it applies. */
if (GET_MODE_BITSIZE (GET_MODE (str_rtx)) > 0)
mode1 = GET_MODE (str_rtx);
was also inserted into store_bit_field.
I also think that, minimally, this needs to be rewritten as something like
if (MEM_P (str_rtx)
&& STRICT_ALIGNMENT
&& GET_MODE_BITSIZE (GET_MODE (str_rtx)) != 0
&& GET_MODE_BITSIZE (GET_MODE (str_rtx)) < GET_MODE_BITSIZE
(word_mode)
&& (bitnum % MEM_ALIGN (str_rtx) + bitsize
> GET_MODE_BITSIZE (GET_MODE (str_rtx))))
str_rtx = adjust_address (str_rtx, word_mode, 0);
Otherwise, we'll end up using word_mode instead of the field mode on
targets that support unaligned accesses. I tested that (again, 4.8 and
arm-none-eabi) and results looked good, but of course ARM isn't one of
the targets that supports unaligned accesses. :-S
I'm still not convinced this is the right fix, though. It seems to me
that callers of store_bit_field and extract_bit_field in expr.c ought
not to be messing with the mode of the rtx when
flag_strict_volatile_bitfields > 0, because that is losing information
about the original mode that we are trying to patch up here by forcing
it to word_mode. Instead, I think the callers ought to be passing the
declared field mode into store_bit_field and extract_bit_field, and
those functions ought to change the mode of the incoming rtx to match
the field mode only if strict_volatile_bitfield_p assures us that the
insertion/extraction can be done in that mode.
Alternatively, if strict_volatile_bitfield_p returns false but
flag_strict_volatile_bitfields > 0, then always force to word_mode and
change the -fstrict-volatile-bitfields documentation to indicate that's
the fallback if the insertion/extraction cannot be done in the declared
mode, rather than claiming that it tries to do the same thing as if
-fstrict-volatile-bitfields were not enabled at all.
Either way, still needs more work, and more thorough testing (more
targets, and obviously trunk as well as the 4.8 backport) before I'd
consider this ready to commit. I might or might not be able to find
some more time to work on this in the next week....
-Sandra
