OK, thanks.
On Wed, Dec 14, 2016 at 5:44 AM, Richard Biener <rguent...@suse.de> wrote:
>
> The following implements Jasons suggestion of using a langhook to
> return the size of an aggregate without tail padding that might
> be re-used when it is inherited from.
>
> Using this langhook we can fix the size of the representative for the
> bitfield properly and thus generate correct code for the testcase.
> Previously on x86_64 it was
>
> main:
> 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
>
> while after the patch we see
>
> main:
> movzbl c+5(%rip), %eax
> andb $-4, c+6(%rip)
> movb $2, c+7(%rip)
> andl $15, %eax
> orl $16, %eax
> movb %al, c+5(%rip)
> xorl %eax, %eax
> ret
>
> in particular the store to C::B::d is now properly using a byte store.
>
> Bootstrap and regtest in progress on x86_64-unknown-linux-gnu, ok for
> trunk?
>
> Thanks,
> Richard.
>
> 2016-12-14 Richard Biener <rguent...@suse.de>
>
> PR c++/71694
> * langhooks-def.h (lhd_unit_size_without_reusable_padding): Declare.
> (LANG_HOOKS_UNIT_SIZE_WITHOUT_REUSABLE_PADDING): Define.
> (LANG_HOOKS_FOR_TYPES_INITIALIZER): Adjust.
> * langhooks.h (struct lang_hooks_for_types): Add
> unit_size_without_reusable_padding.
> * langhooks.c (lhd_unit_size_without_reusable_padding): New.
> * stor-layout.c (finish_bitfield_representative): Use
> unit_size_without_reusable_padding langhook to decide on the
> last representatives size.
>
> cp/
> * cp-objcp-common.h (cp_unit_size_without_reusable_padding): Declare.
> (LANG_HOOKS_UNIT_SIZE_WITHOUT_REUSABLE_PADDING): Define.
> * cp-objcp-common.c (cp_unit_size_without_reusable_padding): New.
>
> * g++.dg/pr71694.C: New testcase.
>
> Index: gcc/langhooks-def.h
> ===================================================================
> *** gcc/langhooks-def.h (revision 243632)
> --- gcc/langhooks-def.h (working copy)
> *************** extern tree lhd_make_node (enum tree_cod
> *** 161,166 ****
> --- 161,168 ----
>
> /* Types hooks. There are no reasonable defaults for most of them,
> so we create a compile-time error instead. */
> + extern tree lhd_unit_size_without_reusable_padding (tree);
> +
> #define LANG_HOOKS_MAKE_TYPE lhd_make_node
> #define LANG_HOOKS_CLASSIFY_RECORD NULL
> #define LANG_HOOKS_INCOMPLETE_TYPE_ERROR lhd_incomplete_type_error
> *************** extern tree lhd_make_node (enum tree_cod
> *** 189,194 ****
> --- 191,197 ----
> #define LANG_HOOKS_GET_DEBUG_TYPE NULL
> #define LANG_HOOKS_GET_FIXED_POINT_TYPE_INFO NULL
> #define LANG_HOOKS_TYPE_DWARF_ATTRIBUTE lhd_type_dwarf_attribute
> + #define LANG_HOOKS_UNIT_SIZE_WITHOUT_REUSABLE_PADDING
> lhd_unit_size_without_reusable_padding
>
> #define LANG_HOOKS_FOR_TYPES_INITIALIZER { \
> LANG_HOOKS_MAKE_TYPE, \
> *************** extern tree lhd_make_node (enum tree_cod
> *** 212,218 ****
> LANG_HOOKS_ENUM_UNDERLYING_BASE_TYPE, \
> LANG_HOOKS_GET_DEBUG_TYPE, \
> LANG_HOOKS_GET_FIXED_POINT_TYPE_INFO, \
> ! LANG_HOOKS_TYPE_DWARF_ATTRIBUTE \
> }
>
> /* Declaration hooks. */
> --- 215,222 ----
> LANG_HOOKS_ENUM_UNDERLYING_BASE_TYPE, \
> LANG_HOOKS_GET_DEBUG_TYPE, \
> LANG_HOOKS_GET_FIXED_POINT_TYPE_INFO, \
> ! LANG_HOOKS_TYPE_DWARF_ATTRIBUTE, \
> ! LANG_HOOKS_UNIT_SIZE_WITHOUT_REUSABLE_PADDING \
> }
>
> /* Declaration hooks. */
> Index: gcc/langhooks.h
> ===================================================================
> *** gcc/langhooks.h (revision 243632)
> --- gcc/langhooks.h (working copy)
> *************** struct lang_hooks_for_types
> *** 166,171 ****
> --- 166,175 ----
> /* Returns -1 if dwarf ATTR shouldn't be added for TYPE, or the attribute
> value otherwise. */
> int (*type_dwarf_attribute) (const_tree, int);
> +
> + /* Returns a tree for the unit size of T excluding tail padding that
> + might be used by objects inheriting from T. */
> + tree (*unit_size_without_reusable_padding) (tree);
> };
>
> /* Language hooks related to decls and the symbol table. */
> Index: gcc/langhooks.c
> ===================================================================
> *** gcc/langhooks.c (revision 243632)
> --- gcc/langhooks.c (working copy)
> *************** lhd_type_dwarf_attribute (const_tree, in
> *** 729,734 ****
> --- 729,743 ----
> return -1;
> }
>
> + /* Default implementation of LANG_HOOKS_UNIT_SIZE_WITHOUT_REUSABLE_PADDING.
> + Just return TYPE_SIZE_UNIT unadjusted. */
> +
> + tree
> + lhd_unit_size_without_reusable_padding (tree t)
> + {
> + return TYPE_SIZE_UNIT (t);
> + }
> +
> /* Returns true if the current lang_hooks represents the GNU C frontend. */
>
> bool
> Index: gcc/stor-layout.c
> ===================================================================
> *** gcc/stor-layout.c (revision 243632)
> --- gcc/stor-layout.c (working copy)
> *************** finish_bitfield_representative (tree rep
> *** 1864,1876 ****
> }
> else
> {
> ! /* ??? If you consider that tail-padding of this struct might be
> ! re-used when deriving from it we cannot really do the following
> ! and thus need to set maxsize to bitsize? Also we cannot
> ! generally rely on maxsize to fold to an integer constant, so
> ! use bitsize as fallback for this case. */
> ! tree maxsize = size_diffop (TYPE_SIZE_UNIT (DECL_CONTEXT (field)),
> ! DECL_FIELD_OFFSET (repr));
> if (tree_fits_uhwi_p (maxsize))
> maxbitsize = (tree_to_uhwi (maxsize) * BITS_PER_UNIT
> - tree_to_uhwi (DECL_FIELD_BIT_OFFSET (repr)));
> --- 1864,1877 ----
> }
> else
> {
> ! /* Note that if the C++ FE sets up tail-padding to be re-used it
> ! creates a as-base variant of the type with TYPE_SIZE adjusted
> ! accordingly. So it is safe to include tail-padding here. */
> ! tree aggsize = lang_hooks.types.unit_size_without_reusable_padding
> ! (DECL_CONTEXT
> (field));
> ! tree maxsize = size_diffop (aggsize, DECL_FIELD_OFFSET (repr));
> ! /* We cannot generally rely on maxsize to fold to an integer constant,
> ! so use bitsize as fallback for this case. */
> if (tree_fits_uhwi_p (maxsize))
> maxbitsize = (tree_to_uhwi (maxsize) * BITS_PER_UNIT
> - tree_to_uhwi (DECL_FIELD_BIT_OFFSET (repr)));
> Index: gcc/cp/cp-objcp-common.c
> ===================================================================
> *** gcc/cp/cp-objcp-common.c (revision 243632)
> --- gcc/cp/cp-objcp-common.c (working copy)
> *************** cp_type_dwarf_attribute (const_tree type
> *** 252,257 ****
> --- 252,267 ----
> return -1;
> }
>
> + /* Return the unit size of TYPE without reusable tail padding. */
> +
> + tree
> + cp_unit_size_without_reusable_padding (tree type)
> + {
> + if (CLASS_TYPE_P (type))
> + return CLASSTYPE_SIZE_UNIT (type);
> + return TYPE_SIZE_UNIT (type);
> + }
> +
> /* Stubs to keep c-opts.c happy. */
> void
> push_file_scope (void)
> Index: gcc/cp/cp-objcp-common.h
> ===================================================================
> *** gcc/cp/cp-objcp-common.h (revision 243632)
> --- gcc/cp/cp-objcp-common.h (working copy)
> *************** extern tree objcp_tsubst_copy_and_build
> *** 30,35 ****
> --- 30,36 ----
> extern int cp_decl_dwarf_attribute (const_tree, int);
> extern int cp_type_dwarf_attribute (const_tree, int);
> extern void cp_common_init_ts (void);
> + extern tree cp_unit_size_without_reusable_padding (tree);
>
> /* Lang hooks that are shared between C++ and ObjC++ are defined here.
> Hooks
> specific to C++ or ObjC++ go in cp/cp-lang.c and objcp/objcp-lang.c,
> *************** extern void cp_common_init_ts (void);
> *** 137,142 ****
> --- 138,146 ----
> #define LANG_HOOKS_DECL_DWARF_ATTRIBUTE cp_decl_dwarf_attribute
> #undef LANG_HOOKS_TYPE_DWARF_ATTRIBUTE
> #define LANG_HOOKS_TYPE_DWARF_ATTRIBUTE cp_type_dwarf_attribute
> + #undef LANG_HOOKS_UNIT_SIZE_WITHOUT_REUSABLE_PADDING
> + #define LANG_HOOKS_UNIT_SIZE_WITHOUT_REUSABLE_PADDING
> cp_unit_size_without_reusable_padding
> +
> #undef LANG_HOOKS_OMP_PREDETERMINED_SHARING
> #define LANG_HOOKS_OMP_PREDETERMINED_SHARING cxx_omp_predetermined_sharing
> #undef LANG_HOOKS_OMP_CLAUSE_DEFAULT_CTOR
> Index: gcc/testsuite/g++.dg/pr71694.C
> ===================================================================
> *** gcc/testsuite/g++.dg/pr71694.C (revision 0)
> --- gcc/testsuite/g++.dg/pr71694.C (working copy)
> ***************
> *** 0 ****
> --- 1,27 ----
> + /* { dg-do compile } */
> + /* { dg-options "-O2" } */
> +
> + struct B {
> + B() {}
> + int x;
> + int a : 6;
> + int b : 6;
> + int c : 6;
> + };
> +
> + struct C : B {
> + char d;
> + };
> +
> + C c;
> +
> + int main()
> + {
> + /* We have to make sure to not cause a store data race between
> + c.c and c.d residing in the tail padding of B. */
> + c.c = 1;
> + c.d = 2;
> + }
> +
> + /* In particular on x86 c.d should not be loaded/stored via movl. */
> + /* { dg-final { scan-assembler-not "movl" { target { x86_64-*-* i?86-*-* }
> } } } */
