On Mon, Jul 25, 2011 at 10:07 AM, Aldy Hernandez <al...@redhat.com> wrote:
> On 07/22/11 13:44, Jason Merrill wrote:
>>
>> On 07/18/2011 08:02 AM, Aldy Hernandez wrote:
>>>
>>> + /* If other threads can't see this value, no need to restrict
>>> stores. */
>>> + if (ALLOW_STORE_DATA_RACES
>>> + || !DECL_THREAD_VISIBLE_P (innerdecl))
>>> + {
>>> + *bitstart = *bitend = 0;
>>> + return;
>>> + }
>>
>> What if get_inner_reference returns something that isn't a DECL, such as
>> an INDIRECT_REF?
>
> I had changed this already to take into account aliasing, so if we get an
> INDIRECT_REF, ptr_deref_may_alias_global_p() returns true, and we proceed
> with the restriction:
>
> + /* If other threads can't see this value, no need to restrict stores. */
> + if (ALLOW_STORE_DATA_RACES
> + || (!ptr_deref_may_alias_global_p (innerdecl)
> + && (DECL_THREAD_LOCAL_P (innerdecl)
> + || !TREE_STATIC (innerdecl))))
>
>
>>> + if (fld)
>>> + {
>>> + /* We found the end of the bit field sequence. Include the
>>> + padding up to the next field and be done. */
>>> + *bitend = bitpos - 1;
>>> + }
>>
>> bitpos is the position of "field", and it seems to me we want the
>> position of "fld" here.
>
> Notice that bitpos gets recalculated at each iteration by
> get_inner_reference, so bitpos is actually the position of fld.
>
>>> + /* If unset, no restriction. */
>>> + if (!bitregion_end)
>>> + maxbits = 0;
>>> + else
>>> + maxbits = (bitregion_end - bitregion_start) % align;
>>
>> Maybe use MAX_FIXED_MODE_SIZE so you don't have to test it against 0?
>
> Fixed everywhere.
>
>>> + if (!bitregion_end)
>>> + maxbits = 0;
>>> + else if (1||bitpos + offset * BITS_PER_UNIT < bitregion_start)
>>> + maxbits = bitregion_end - bitregion_start;
>>> + else
>>> + maxbits = bitregion_end - (bitpos + offset * BITS_PER_UNIT) + 1;
>>
>> I assume the 1|| was there for debugging?
>
> Fixed, plus I adjusted the calculation of maxbits everywhere because I found
> an off-by-one error.
>
> I have also overhauled store_bit_field() to adjust the address of the
> address to point to the beginning of the bit region. This fixed a myraid of
> corner cases pointed out by a test Hans Boehm was kind enough to provide.
>
> I have added more tests.
>
> How does this look? (Pending tests.)
>
This caused:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49875
--
H.J.
