Re: [C++0x] contiguous bitfields race implementation

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Jul 27, 2011 at 4:56 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, Jul 27, 2011 at 4:52 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Tue, Jul 26, 2011 at 7:38 PM, Jason Merrill <jason@redhat.com> wrote:
>>> On 07/26/2011 10:32 AM, Aldy Hernandez wrote:
>>>>
>>>>> I think the adjustment above is intended to match the adjustment of the
>>>>> address by bitregion_start/BITS_PER_UNIT, but the above seems to assume
>>>>> that bitregion_start%BITS_PER_UNIT == 0.
>>>>
>>>> That was intentional. bitregion_start always falls on a byte boundary,
>>>> does it not?
>>>
>>> Ah, yes, of course, it's bitnum that might not. ?The code changes look good,
>>> then.
>>
>> Looks like this was an approval ...
>>
>> Anyway, I don't think a --param is appropriate to control a flag whether
>> to allow store data-races to be created. ?Why not use a regular option instead?
>>
>> I believe that any after-the-fact attempt to recover bitfield boundaries is
>> going to fail unless you preserve more information during bitfield layout.
>>
>> Consider
>>
>> struct {
>> ?char : 8;
>> ?char : 0;
>> ?char : 8;
>> };
>>
>> where the : 0 isn't preserved in any way and you can't distinguish
>> it from struct { char : 8; char : 8; }.
>
> Oh, and
>
> ? INNERDECL is the actual object being referenced.
>
> ? ? ?|| (!ptr_deref_may_alias_global_p (innerdecl)
>
> is surely not what you want. ?That asks if *innerdecl is global memory.
> I suppose you want is_global_var (innerdecl)? ?But with
>
> ? ? ? ? ?&& (DECL_THREAD_LOCAL_P (innerdecl)
> ? ? ? ? ? ? ?|| !TREE_STATIC (innerdecl))))
>
> you can simply skip this test. ?Or what was it supposed to do?

And

      t = build3 (COMPONENT_REF, TREE_TYPE (exp),
                  unshare_expr (TREE_OPERAND (exp, 0)),
                  fld, NULL_TREE);
      get_inner_reference (t, &bitsize, &bitpos, &offset,
                           &mode, &unsignedp, &volatilep, true);

for each field of a struct type is of course ... gross!  In fact you already
have the FIELD_DECL in the single caller!  Yes I know there is not
enough information preserved by bitfield layout - see my previous reply.

      if (TREE_CODE (to) == COMPONENT_REF
          && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
        get_bit_range (&bitregion_start, &bitregion_end,
                       to, tem, bitpos, bitsize);

and shouldn't this test DECL_BIT_FIELD instead of DECL_BIT_FIELD_TYPE?

Richard.