Re: Adjust empty class parameter passing ABI (PR c++/60336)

[Uros Bizjak <ubizjak at gmail dot com>]

On Mon, Nov 20, 2017 at 4:51 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Thu, Nov 16, 2017 at 02:20:59PM -0500, Jason Merrill wrote:
>> On Thu, Nov 16, 2017 at 12:41 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > On Tue, Nov 14, 2017 at 07:34:54AM +0100, Richard Biener wrote:
>> >> On November 14, 2017 6:21:41 AM GMT+01:00, Jason Merrill <jason@redhat.com> wrote:
>> >> >On Mon, Nov 13, 2017 at 1:02 PM, Marek Polacek <polacek@redhat.com>
>> >> >> In the end I did two bootstraps with the patch, but modifed one of
>> >> >them
>> >> >> to always return false for ix86_is_empty_record.  Then I compared all
>> >> >the
>> >> >> *.o in both dirs.  The result is attached.  Then I looked at
>> >> >DW_AT_producer
>> >> >> for all these .o that differ; all of them are C++.  Is this enough to
>> >> >> clear our concerns?
>> >> >
>> >> >Hmm, a bunch of these are right at the beginning, bytes 41 and 65, in
>> >> >the header.
>> >> >
>> >> >Did you build them in the different trunk/trunk2 directories?  I think
>> >> >Jakub was suggesting building them in the same directory.
>> >> >> And I also ran a bootstrap with --enable-cxx-flags=-Wabi=11, and
>> >> >didn't
>> >> >> see any warnings.
>> >> >
>> >> >If there's a codegen change, there ought to be a warning to go along
>> >> >with it.
>> >>
>> >> The question was of course also for unintended changes but yes (I was mainly concerned by libstdc++ ABI changes).
>> >
>> > Ok, I did two bootstraps in the same dir, one with ix86_is_empty_record always
>> > returning false.  There were a few object files that differ in their assembly
>> > between those two bootstraps.  Previously I didn't see any warnings because
>> > I hadn't thought of -Wsystem-headers.  Also, we intentionally don't warn if
>> > the empty parameter is the last one:
>> >
>> > +      bool seen_empty_type = false;
>> > +      FOREACH_FUNCTION_ARGS (fntype, argtype, iter)
>> > +       {
>> > +         if (VOID_TYPE_P (argtype))
>> > +           break;
>> > +         if (TYPE_EMPTY_P (argtype))
>> > +           seen_empty_type = true;
>> > +         else if (seen_empty_type)
>> > +           {
>> > +             cum->warn_empty = true;
>> > +             break;
>> > +           }
>> > +       }
>> >
>> > After enabling -Wsystem-headers and tweaking the code above so that we warn
>> > even if the empty parameter is trailing I can see the warnings that correspond
>> > to the assembly changes.  Below is a summary of what I found.  TL;DR: I don't
>> > see any unintended changes.
>>
>> Looks good to me.
>
> Thanks!
>
> Richi, are you ok with the patch now?
> Honza/Uros, are the config/i386/* changes ok?
>
> The last version of the patch is
> https://gcc.gnu.org/ml/gcc-patches/2017-11/msg00969.html

LGTM for x86 part.

Thanks,
Uros.