Re: [Qemu-devel] [PATCH v8 03/13] dump: add API to write header of flatten format

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> On 02/10/14 23:48, Luiz Capitulino wrote:
>> On Mon, 10 Feb 2014 22:57:15 +0100
>> Laszlo Ersek <address@hidden> wrote:
>
>>> In short, -Werror and bisection don't mix. They already don't, and we
>>> shouldn't expect them to.
>> 
>> I understand what you're saying, and I don't want people to do needless and
>> endless respins, but letting bisect break at will doesn't seem a good option
>> either.
>> 
>> What other options do we have? What's the general QEMU directive in cases 
>> like
>> this?
>> 
>>  1. Do what you did in commit 27d59ccd?
>> 
>>  2. Apply it and let bisect break?
>> 
>>  3. Drop -Werror?
>
> In commit 27d59ccd, I introduced the create_blob_file() function, in
> "tests/i440fx-test.c". The patch was really about nothing else than
> introducing this function (which would have made no sense outside of
> said C file, so I made it static).
>
> (The new function was to be utilized in the next patch (3bcc77ae).)
>
> Of course gcc whined at 27d59ccd, and I was forced to call
> create_blob_file() from main() just to shut it up. This function call
> made absolutely no sense. I only added the call in order to convince gcc
> that create_blob_file() was not some abandoned, useless function.
>
> Of course, when the tree is built at 27d59ccd, *and* "tests/i440fx-test"
> is run, then create_blob_file() runs too, and it *has* an effect. A blob
> file is indeed created and unlinked. In other words, in that commit the
> utility function is actually exercised, *outside* its intended
> scope/context, just to shut up gcc.
>
> To follow suit, Qiao would have to call write_start_flat_header() and
> write_end_flat_header() *somewhere* in patch v8 03/13. It would be a
> terrible thing to do. It makes absolutely no sense to call these
> functions in this patch. And I don't think we could trick gcc with an
> "if (0) {...}", because the optimizer would eliminate the call and we'd
> be left with the original warning/error.
>
> Summary: in commit 27d59ccd, I did a horrible hack to shut up gcc. It
> was only permissible because the hack affected just a test case. It
> didn't affect production code that you actually might want to bisect.

Ugh!

> The general qemu (and edk2) approach is to rape the code until the
> *contemporary* gcc / compiler of choice shuts up. Then, at *real* bisect
> time later down the road, with a new compiler release, act surprised
> when the tree doesn't build. Then repeat/restart the bisection with
> -Werror disabled.

I always configure --disable-werror.  It doesn't make the sky fall.

> My specific proposal is to test-build the tree at all patches in the
> series, *except* at the last one, with -Werror disabled. Then build the
> tree at the final patch with -Werror enabled.

My proposal is a generalization of yours: use a bit of common sense for
a change :)