[PATCH][C++] C++ bitfield memory model for as-base classes

[Richard Biener]

On Wed, 20 Jul 2016, Jeff Law wrote:

> On 06/29/2016 05:54 AM, Richard Biener wrote:
> > 
> > Currently as-base classes lack DECL_BIT_FIELD_REPRESENTATIVEs which
> > means RTL expansion doesn't honor the C++ memory model for bitfields
> > in them thus for the following testcase
> > 
> > struct B {
> >     B() {}
> >     int x;
> >     int a : 6;
> >     int b : 6;
> >     int c : 6;
> > };
> > 
> > struct C : B {
> >     char d;
> > };
> > 
> > C c;
> > 
> > int main()
> > {
> >   c.c = 1;
> >   c.d = 2;
> > }
> > 
> > on x86 we happily store to c.c in a way creating a store data race with
> > c.d:
> > 
> > main:
> > .LFB6:
> >         .cfi_startproc
> >         movl    c+4(%rip), %eax
> >         andl    $-258049, %eax
> >         orb     $16, %ah
> >         movl    %eax, c+4(%rip)
> >         movb    $2, c+7(%rip)
> >         xorl    %eax, %eax
> >         ret
> > 
> > Fixing the lack of DECL_BIT_FIELD_REPRESENTATIVEs in as-base
> > classes doesn't help though as the C++ FE builds access trees
> > for c.c using the non-as-base class FIELD_DECLs which is because
> > of layout_class_type doing
> > 
> >   /* Now that we're done with layout, give the base fields the real types.
> > */
> >   for (field = TYPE_FIELDS (t); field; field = DECL_CHAIN (field))
> >     if (DECL_ARTIFICIAL (field) && IS_FAKE_BASE_TYPE (TREE_TYPE (field)))
> >       TREE_TYPE (field) = TYPE_CONTEXT (TREE_TYPE (field));
> > 
> > this would basically require us to always treat tail-padding in a
> > struct conservatively in finish_bitfield_representative (according
> > to the doubt by the ??? comment I patch out below).
> > 
> > Simply commenting out the above makes fixing build_simple_base_path
> > necessary but even after that it then complains in the verifier
> > later ("type mismatch in component reference" - as-base to class
> > assignment).
> > 
> > But it still somehow ends up using the wrong FIELD_DECL in the end.
> > 
> > Now I think we need to fix the wrong-code issue somehow and
> > doing so in stor-layout.c by conservatively treating tail-padding
> > is a possibility.  But that will pessimize PODs and other languages
> > unless we have a way to know whether a RECORD_TYPE possibly can
> > have its tail-padding re-used (I'd hate to put a lang_hooks.name
> > check there and even that would pessimize C++ PODs).
> > 
> > Any guidance here?
> Note that if we change tail-padding re-use properties, then we effectively
> have an ABI change.  Given that, the only path forward is to use smaller
> memory operations.

Yes, we can't change the ABI.  It seems the most straigt-forward solution
is to add a langhook we can query from stor-layout that says whether
tail padding of an aggregate might be re-used.

> Do any other compilers gets this right (LLVM?)

clang++ 3.6.1 gives me

        movzbl  c+6(%rip), %eax
        shll    $16, %eax
        movzwl  c+4(%rip), %ecx
        orl     %eax, %ecx
        andl    $16519167, %ecx         # imm = 0xFC0FFF
        leal    4096(%rcx), %eax
        shrl    $16, %ecx
        movb    %cl, c+6(%rip)
        movw    %ax, c+4(%rip)
        movb    $2, c+7(%rip)
        xorl    %eax, %eax
        retq

so it avoids the store data race (it even avoids load data races).
Not sure if it does because of the C++ memory model or lack of
optimization.

Richard.
 
> jeff
> 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)