This is the mail archive of the gcc-patches@gcc.gnu.org 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


Hi Sandra,


On Sun, 20 Oct 2013 20:23:49, Sandra Loosemore wrote:
>
> On 10/18/2013 10:38 AM, Richard Biener wrote:
>> Sandra Loosemore <sandra@codesourcery.com> wrote:
>>> On 10/18/2013 04:50 AM, Richard Biener wrote:
>>>> On Sat, Sep 28, 2013 at 4:19 AM, Sandra Loosemore
>>>> <sandra@codesourcery.com> wrote:
>>>>> This patch fixes various -fstrict-volatile-bitfields wrong-code
>>> bugs,
>>>>> including instances where bitfield values were being quietly
>>> truncated as
>>>>> well as bugs relating to using the wrong width. The code changes
>>> are
>>>>> identical to those in the previous version of this patch series
>>> (originally
>>>>> posted in June and re-pinged many times since then), but for this
>>> version I
>>>>> have cleaned up the test cases to remove dependencies on header
>>> files, as
>>>>> Bernd requested.
>>>>
>>>> Ok.
>>>
>>> Just to clarify, is this approval unconditional, independent of part 2
>>> and other patches or changes still under active discussion?
>>
>> Yes.
>
> Hrmmmm. After some further testing, I'm afraid this patch is still
> buggy. :-(
>
> 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.
>
> The difference between 4.8 and mainline head is the alignment of the
> incoming str_rtx passed to store_bit_field/extract_bit_field, due to the
> changes in r199898. The alignment is 8 bits on mainline, and 32 on 4.8.
> It seems to me that the bitfield code ought to handle store/extract
> from a more-aligned object and it's probably possible to construct an
> example that fails in this way on mainline as well.
>
> It looks like there are conflicting assumptions between get_best_mode,
> the places that call it in store_fixed_bit_field and
> extract_fixed_bit_field, and the code that actually does the splitting
> -- which uses a unit based on the MEM_ALIGN of the incoming rtx, rather
> than its mode. In the case where it's failing, get_best_mode is
> deciding it can't do a HImode access without splitting, but then the
> split code is assuming SImode units because of the 32-bit alignment, but
> not actually changing the mode of the rtx to match that.
>
> On top of that, this is one of the cases that strict_volatile_bitfield_p
> checks for and returns false, but the callers of store_bit_field and
> extract_bit_field in expr.c have already fiddled with the mode of the
> incoming rtx assuming that -fstrict-volatile-bitfields does apply. It
> doesn't get into that infinite recursion if it's compiled with
> -fno-strict-volatile-bitfields instead; in that case the incoming rtx
> has BLKmode, get_best_mode chooses SImode, and it's able to do the
> access without splitting at all.
>
> Anyway.... I tried a couple different bandaids that solved the infinite
> recursion problem but caused regressions elsewhere, and now I'm not sure
> of the right place to fix this. Given that there is also still ongoing
> discussion about making this a 3-way switch (etc) I am going to hold off
> on committing this patch for now.
>
> -Sandra
>

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.

Bernd. 		 	   		  

Attachment: patch-bitfields-update.diff
Description: Binary data


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