PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class

On Wed, Dec 9, 2015 at 10:53 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Dec 9, 2015 at 6:05 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Tue, Dec 8, 2015 at 5:22 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Mon, Nov 23, 2015 at 12:53 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Mon, Nov 23, 2015 at 1:57 AM, Richard Biener
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Sat, Nov 21, 2015 at 12:46 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>> On Fri, Nov 20, 2015 at 2:17 PM, Jason Merrill <jason@redhat.com> wrote:
>>>>>>> On 11/20/2015 01:52 PM, H.J. Lu wrote:
>>>>>>>>
>>>>>>>> On Tue, Nov 17, 2015 at 4:22 AM, Richard Biener
>>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Nov 17, 2015 at 12:01 PM, H.J. Lu <hjl.tools@gmail.com> 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.
>

Here is a patch for other backends to properly pass C++ empty class.
One can check out hjl/pr60336/master branch from

https://github.com/hjl-tools/gcc

to get both generic and target-dependent patches.

PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class

Commit Message

Patch