linux-kernel - Re: [tip:x86/build] x86/build: Remove -Wno-sign-compare

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]

Message-id: <E40D0A5D-A591-48E1-A011-339FAECCAA43@me.com>
Date:	Wed, 13 May 2015 11:33:20 -0600
From:	Louis Langholtz <lou_langholtz@...com>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Valdis.Kletnieks@...edu,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Peter Anvin <hpa@...or.com>, Brian Gerst <brgerst@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Borislav Petkov <bp@...en8.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Andy Lutomirski <luto@...capital.net>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [tip:x86/build] x86/build: Remove -Wno-sign-compare

On May 13, 2015, at 4:22 AM, Ingo Molnar <mingo@...nel.org> wrote:

> * Valdis.Kletnieks@...edu <Valdis.Kletnieks@...edu> wrote:
> 
>> On Tue, 12 May 2015 10:44:15 +0200, Ingo Molnar said:
>> 
>>> ...
>>> Before I pushed out this -Wno-sign-compare change I made sure there
>>> are no extra warnings generated on the 8 key configs I monitor
>>> explicitly: [def|allno|allyes|allmod]config[64|32].
>> 
>> It makes my config totally explode on next-20150512

I note that not all architectures add the no-sign-compare option to their builds.
Specifically (according to grep of arch/*/Makefile), s390, sparc, and x86 do add
this option while none of the others do (well alpha may but not in that same
Makefile).

Are warnings about sign-comparisons an architecture specific issue? I can
imagine that for some architectures the sign comparison is a bigger concern
than for others. I can also imagine that this could be architecture independent
enough to warrant being considered an option for all the kernel builds. Do devs
for the other architectures just live with all these warnings? Or 'clean' them up?

>> 
>> % gcc --version
>> gcc (GCC) 5.1.1 20150422 (Red Hat 5.1.1-1)
>> 
>> % grep "warning: comparison between signed and unsigned integer expressions" build.default | wc -l
>> 64148
>> 
>> A *lot*of them appear to be exploding inside likely().  Here's all the
>> exploding for *one file*...
> 

A lot of the explosion appears to be due to the sign compare issue being in
code that's inline or macro code in header files that get included in multiple C
files. (Is that what you're saying in your next sentence?)

> That's a prerelease of GCC, right? So I think GCC gets constant 
> propagation from various builtins wrong or so.

GCC seems to issue a warning on every use of code; not just the first. I won't
call that wrong. It does decrease the signal-to-noise ratio however and in that
sense I do find it less desirable. OTOH, that does help to prioritize include files
that are more responsible for sign-compare issues.

> 
> But ... the output looks horrible, and for years we didn't have the 
> warnings, still after reintroducing it we didn't get any new warnings 
> about truly bogus code.

I'd hate to have to go through every sign compare that was flagged to see
whether the mixed sign-unsigned comparison actually introduced a potential
real problem. OTOH, there's a lot of places as we're seeing where the
comparison occurs and a lot of these places don't appear to have defensive
code to handle a potentially real comparison flaw.

> So it does not seem to have much utility, and seems to be horribly 
> broken on fresh GCC.

I use gcc 4.9.2 and I see the above described explosion (so not just gcc 5.1.1+).

> 
> I'll remove the commit.
> 
> Thanks,
> 
> 	Ingo

When I submitted the patch that I did (that addresses sign comparisons in
the arch/x86/include/asm/uaccess.h file) that appears to have started this thread,
I considered submitting the change to the Makefile (like you made). While I'd love
to see this change *someday*, I decided against it for now because it seemed like
this would introduce a more significant change that requires a lot more analysis
that probably isn't worth the trouble. Individual developers can remove the
option and address flagged code in the meantime without bothering more people
that aren't interested in potential sign comparison issues in other people's code.

I believe Linus brought up the concern too about how sign-unsigned comparisons
might get addressed. I think there's sometimes a clear-cut correct "fix" but often
there isn't. So I could see a lot of patches coming in to quiet the compiler that also
caused more problems like hiding problems that would be better addressed more
comprehensively.

Lou--
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/