On Thu, Nov 16, 2017 at 02:20:59PM -0500, Jason Merrill wrote:
> On Thu, Nov 16, 2017 at 12:41 PM, Marek Polacek <pola...@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
> >> <ja...@redhat.com> wrote:
> >> >On Mon, Nov 13, 2017 at 1:02 PM, Marek Polacek <pola...@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
Marek
