On Wed, Dec 9, 2015 at 6:05 AM, Richard Biener wrote: > On Tue, Dec 8, 2015 at 5:22 PM, H.J. Lu wrote: >> On Mon, Nov 23, 2015 at 12:53 PM, H.J. Lu wrote: >>> On Mon, Nov 23, 2015 at 1:57 AM, Richard Biener >>> wrote: >>>> On Sat, Nov 21, 2015 at 12:46 AM, H.J. Lu wrote: >>>>> On Fri, Nov 20, 2015 at 2:17 PM, Jason Merrill wrote: >>>>>> On 11/20/2015 01:52 PM, H.J. Lu wrote: >>>>>>> >>>>>>> On Tue, Nov 17, 2015 at 4:22 AM, Richard Biener >>>>>>> wrote: >>>>>>>> >>>>>>>> On Tue, Nov 17, 2015 at 12:01 PM, H.J. Lu wrote: >>>>>>>>> >>>>>>>>> Empty record should be returned and passed the same way in C and C++. >>>>>>>>> This patch adds LANG_HOOKS_EMPTY_RECORD_P for C++ empty class, which >>>>>>>>> defaults to return false. For C++, LANG_HOOKS_EMPTY_RECORD_P is defined >>>>>>>>> to is_really_empty_class, which returns true for C++ empty classes. For >>>>>>>>> LTO, we stream out a bit to indicate if a record is empty and we store >>>>>>>>> it in TYPE_LANG_FLAG_0 when streaming in. get_ref_base_and_extent is >>>>>>>>> changed to set bitsize to 0 for empty records. Middle-end and x86 >>>>>>>>> backend are updated to ignore empty records for parameter passing and >>>>>>>>> function value return. Other targets may need similar changes. >>>>>>>> >>>>>>>> >>>>>>>> Please avoid a new langhook for this and instead claim a bit in >>>>>>>> tree_type_common >>>>>>>> like for example restrict_flag (double-check it is unused for >>>>>>>> non-pointers). >>>>>>> >>>>>>> >>>>>>> There is no bit in tree_type_common I can overload. restrict_flag is >>>>>>> checked for non-pointers to issue an error when it is used on >>>>>>> non-pointers: >>>>>>> >>>>>>> >>>>>>> /export/gnu/import/git/sources/gcc/gcc/testsuite/g++.dg/template/qualttp20.C:19:38: >>>>>>> error: ‘__restrict__’ qualifiers cannot be applied to ‘AS::L’ >>>>>>> typedef typename T::L __restrict__ r;// { dg-error "'__restrict__' >>>>>>> qualifiers cannot" "" } >>>>>> >>>>>> >>>>>> The C++ front end only needs to check TYPE_RESTRICT for this purpose on >>>>>> front-end-specific type codes like TEMPLATE_TYPE_PARM; cp_type_quals could >>>>>> handle that specifically if you change TYPE_RESTRICT to only apply to >>>>>> pointers. >>>>>> >>>>> >>>>> restrict_flag is also checked in this case: >>>>> >>>>> [hjl@gnu-6 gcc]$ cat x.i >>>>> struct dummy { }; >>>>> >>>>> struct dummy >>>>> foo (struct dummy __restrict__ i) >>>>> { >>>>> return i; >>>>> } >>>>> [hjl@gnu-6 gcc]$ gcc -S x.i -Wall >>>>> x.i:4:13: error: invalid use of ‘restrict’ >>>>> foo (struct dummy __restrict__ i) >>>>> ^ >>>>> x.i:4:13: error: invalid use of ‘restrict’ >>>>> [hjl@gnu-6 gcc]$ >>>>> >>>>> restrict_flag can't also be used to indicate `i' is an empty record. >>>> >>>> I'm sure this error can be done during parsing w/o relying on TYPE_RESTRICT. >>>> >>>> But well, use any other free bit (but do not enlarge >>>> tree_type_common). Eventually >>>> you can free up a bit by putting sth into type_lang_specific currently >>>> using bits >>>> in tree_type_common. >>> >>> There are no bits in tree_type_common I can move. Instead, >>> this patch overloads side_effects_flag in tree_base. Tested on >>> Linux/x86-64. OK for trunk? > > I'm fine with using side_effects_flag for this. > > I miss an explanation of where this detail of the ABI is documented and wonder > if the term "empty record" is also used in that document and how it is > documented. > > Thus > > +/* Nonzero in a type considered an empty record. */ > +#define TYPE_EMPTY_RECORD(NODE) \ > + (TYPE_CHECK (NODE)->base.side_effects_flag) > > should refer to the ABI where is defined what an "empty record" is and how > it is handled by the backend(s). Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI. There is no mention of "empty record" in GCC documentation. But there are plenty of "empty class" in gcc/cp. This change affects all targets. C++ ABI should specify how it should be passed. > +/* Return true if type T is an empty record. */ > + > +static inline bool > +type_is_empty_record_p (const_tree t) > +{ > + return TYPE_EMPTY_RECORD (TYPE_MAIN_VARIANT (t)); > > the type checker should probably check the bit is consistent across > variants so it can be tested on any of them. TYPE_EMPTY_RECORD is only relevant for parameter passing. For struct foo; typedef foo bar; TYPE_EMPTY_RECORD has no impact. Since call only uses the main variant type, checking TYPE_MAIN_VARIANT is sufficient. > You fail to adjust other targets gimplification hooks which suggests > this is a detail of the x86 psABI and not the C++ ABI? If so I miss > a -Wpsabi warning for this change. My change is generic. The only x86 specific change is diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index d30fbff..308d9a4e 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -10292,7 +10292,8 @@ ix86_gimplify_va_arg (tree valist, tree type, gimple_seq *pre_p, indirect_p = pass_by_reference (NULL, TYPE_MODE (type), type, false); if (indirect_p) type = build_pointer_type (type); - size = int_size_in_bytes (type); + bool empty_record = type && type_is_empty_record_p (type); + size = empty_record ? 0 : int_size_in_bytes (type); rsize = CEIL (size, UNITS_PER_WORD); nat_mode = type_natural_mode (type, NULL, false); which is for 64-bit variadic functions. I added a testcase, g++.dg/pr60336-2.C, for it. There are also other variadic tests in g++.dg/compat. If a target doesn't use std_gimplify_va_arg_expr, it needs similar change. I see that TARGET_GIMPLIFY_VA_ARG_EXPR is also defined in aarch64/aarch64.c alpha/alpha.c arm/arm.c epiphany/epiphany.c ia64/ia64.c mep/mep.c mips/mips.c msp430/msp430.c pa/pa.c rs6000/rs6000.c s390/s390.c sh/sh.c sparc/sparc.c spu/spu.c stormy16/stormy16.c tilegx/tilegx.c tilepro/tilepro.c visium/visium.c xtensa/xtensa.c I will prepare a separate patch for those targets to try. > Did you investigate the effect of this patch on a larger code base? I use it to bootstrap GCC. > More specifically does it only affect variadic functions? See above. > An entry for changes.html is warranted as well. > > I will prepare a patch for changes.html. Here is the updated patch with -Wpsabi warning: /export/gnu/import/git/sources/gcc/gcc/testsuite/g++.dg/abi/empty12.C:15:12: note: the ABI of passing empty record has changed in GCC 6 Thanks. -- H.J.