Date:	Tue, 23 Sep 2014 20:43:17 +0000
From:	"Rustad, Mark D" <mark.d.rustad@...el.com>
To:	Borislav Petkov <bp@...en8.de>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"sparse@...isli.org" <sparse@...isli.org>,
	"linux-sparse@...r.kernel.org" <linux-sparse@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/7] Silence even more W=2 warnings

On Sep 23, 2014, at 11:44 AM, Borislav Petkov <bp@...en8.de> wrote:

> On Tue, Sep 23, 2014 at 05:24:22PM +0000, Rustad, Mark D wrote:
>> Yes, but I think there are a few cases where it could be helpful. When
>> there is something exceptional that will throw a warning. In one of the
>> patches that Jeff sent, I used the DIAG_CLANG_IGNORE macro to suppress
>> the warning that is thrown for every entry of the syscall table when
>> compiled with clang. The code is right and doing exactly what is wanted,
>> so note the exception and make it shut up.
> 
> So we're doing some dancing around solely to shut up the compiler? Even
> if the code is correct?!? Sorry, this is just ass backwards.

Well, please consider the specifics. The entire syscall table is initialized
with a constant pattern to be sure that every item is initialized. Then each
syscall is initialized into its proper place. The compiler is complaining that
entries are being initialized twice.

Most of the time, that is not done, and so it may catch a patch foulup or
something. In this particular case, it is normal and intended. There is
nothing wrong, so there is no reason to throw a warning for every single
entry in the table. Which is what happens with clang today.

So the code is correct, but in general the warning can reveal certain issues.
Just not in this particular usage. This happens to be a warning specific to
clang at the moment.

> If it were me, I'd say even one is too much. Because the very thing
> of adding code just to shut up the compiler which generates otherwise
> correct code is simply very very wrong in my book.
> 
> Bear in mind that even if initially you have a low number of macro
> invocations, that number will grow because people will start using it in
> other places too. And all of a sudden, the thing has spread like weed
> and there's no removing it anymore. So we better not start in the first
> place.

That is why it would be more than reasonable for checkpatch to warn on the
macro introductions. It is certainly a more significant thing than a
line > 80 characters.

> Again, we should take compiler warnings with a grain of salt and judge
> them only by the quality of the generated code. IMO.

I think more than the nature of the executable code matters. The namespace
issues revealed by -Wshadow can certainly create nasty surprises over time.
But we can't get that value from them when they are lost in an ocean of
warnings that are always there and are not the source of any trouble.

The problem is that when so many warnings are generated, particularly by
include files, even useful warnings will not be seen. Specifically
silencing ones that are deemed to be "ok" will help in seeing ones that
are not. The silencing has the greatest effect in the include files,
since there is such a multiplier effect.

Warnings that no one looks at, or bother to generate at all, have
absolutely no value. Even a silenced warning continues to wear its
shame attribute (macro). Hmm. Maybe instead of DIAG_* they should be
named SHAME_*.

Most of the time, it is new instances of warnings that are most likely to
reveal a problem. Hiding them in a flood of "normal" warnings prevents
them from ever being seen. And that is a shame.

-- 
Mark Rustad, Networking Division, Intel Corporation


Download attachment "signature.asc" of type "application/pgp-signature" (842 bytes)

Powered by blists - more mailing lists