On Mon, Nov 20, 2017 at 4:51 PM, Marek Polacek <pola...@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 <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
LGTM for x86 part.
Thanks,
Uros.
