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

[Richard Biener <richard.guenther@gmail.com>]

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).

+/* 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.

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.

Did you investigate the effect of this patch on a larger code base?
More specifically does it only affect variadic functions?

An entry for changes.html is warranted as well.

Thanks,
Richard.

>> Thanks.
>>
>> --
>> H.J.
>
>
>
> --
> H.J.