linux-kernel - Re: [PATCH 0/7] Silence even more W=2 warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 23 Sep 2014 10:01:20 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:	"Rustad, Mark D" <mark.d.rustad@...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 Mon, Sep 22, 2014 at 02:21:52PM -0700, Jeff Kirsher wrote:
> Nothing is wrong with grepping for an error, especially when you know
> the error your grepping for.  But then again, why grep when it can be
> fixed to begin with?

Oh sure, but at what cost?

But we're on the same page here - if it can be fixed cleanly, it should
be fixed. I simply don't think that adding code just to shut up the
compiler is a good idea.

> The fact that there are over 100,000 warnings/errors to begin with
> is somewhat disconcerting. It makes me wonder whether it was due to
> coding laziness.

Yeah, the reasons should be multiple and scattered over the years, I
think. Maybe the warnings were added to gcc later than the code in the
kernel, or simply some of them are just silly:

./arch/x86/include/asm/io_apic.h: In function ‘io_apic_modify’:
./arch/x86/include/asm/io_apic.h:223:48: warning: declaration of ‘apic’ shadows a global declaration [-Wshadow]
 static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned int value)
                                                ^
In file included from ./arch/x86/include/asm/smp.h:12:0,
                 from include/linux/smp.h:59,
                 from include/linux/topology.h:33,
                 from include/linux/gfp.h:8,
                 from include/linux/kmod.h:22,
                 from include/linux/module.h:13,
                 from drivers/edac/amd64_edac.h:65,
                 from drivers/edac/amd64_edac.c:1:
./arch/x86/include/asm/apic.h:366:21: warning: shadowed declaration is here [-Wshadow]
 extern struct apic *apic;
                     ^

So gcc complains that an unsigned int shadows a struct apic pointer.
Yeah, that might be a problem in a big function (and it has been a
problem AFAIR) but here:

static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned int value)
{
        x86_io_apic_ops.modify(apic, reg, value);
}

So yeah, it is debatable. Should it be fixed? Sure, the fix is very easy.

But then fixing those could become a fighting windmills type of thing
as people don't see those warnings in normal builds. And new code will
get added which causes them again. And then the discussion would start
whether we should see those warnings...

I think you get the picture.

> To make a better (more solid) network driver?  Mark has found it useful
> to do the W= builds.  For me personally, I do not bother because there
> are over 100,000 warnings and it takes forever to get through a build
> and then grep for our drivers to see if they are generating any
> warnings.

Right.

> I could see this useful if there is no way to fix the issue that really
> is not an issue and the compiler just does not know any better, but this
> concerns me that we would get into a bad habit.  "Oh I really do not
> want to fix this, so I will just make it so that people we not have to
> see this warning/error"  Again, sounds lazy to me, of course I am just
> speaking in a generalization.

It doesn't have to be lazy - people simply don't see those warnings and
making them visible might turn out to be a net loss in the end. I think
it depends on the warning...

> I am sure that some of the warnings will fall into the category of,
> it needs to be silenced and not fixed because the fix is far more
> troublesome. I just cannot believe that most or all the warnings would
> be that way.

As I said above, I think it is a question of whether those should be
visible/enabled (which would make people fix them, more or less) or not.
And I think if someone comes up with a strong case for why a warning
should be enabled in the default build, then people wouldn't mind.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/