This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] reimplement -fstrict-volatile-bitfields v4, part 1/2

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.


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)
      && 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....


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]