* Linus Torvalds <[email protected]> wrote:
> On May 11, 2015 05:44, "tip-bot for Ingo Molnar" <[email protected]> wrote:
> >
> > So restore the warning and see what happens.
>
> The gcc sign compare had been *totally* broken, I really don't want
> to get a "let's see what happens" thing.
>
> I do "allmodconfig" builds all the time, and actually check
> warnings. [...]
Yeah, so what I failed to point out in the changelog is that I started
doing that too a couple of weeks ago in my continuous integration
kernel testing setup. There is a "baseline warnings count" gathered
from your tree, so I can monitor changes in the level of warnings
emitted:
def64: # 1, 8e70122d, Tue_May_12_09_38_25_CEST_2015: 0 kernels/hour, [ bzImage... 32 secs, modules... 4 secs, done ] (warns: 0)
def32: # 2, 8e70122d, Tue_May_12_09_38_47_CEST_2015: 156 kernels/hour, [ bzImage... 30 secs, modules... 3 secs, done ] (warns: 0)
allno64: # 3, 8e70122d, Tue_May_12_09_39_24_CEST_2015: 120 kernels/hour, [ bzImage... 22 secs, modules... 0 secs, done ] (warns: 2)
allno32: # 4, 8e70122d, Tue_May_12_09_39_58_CEST_2015: 114 kernels/hour, [ bzImage... 21 secs, modules... 0 secs, done ] (warns: 1)
allyes64: # 5, 8e70122d, Tue_May_12_09_40_21_CEST_2015: 123 kernels/hour, [ bzImage... 204 secs, modules... 9 secs, done ] (warns: 10)
allyes32: # 6, 8e70122d, Tue_May_12_09_40_44_CEST_2015: 128 kernels/hour, [ bzImage... 206 secs, modules... 9 secs, done ] (warns: 16)
allmod64: # 7, 8e70122d, Tue_May_12_09_44_19_CEST_2015: 60 kernels/hour, [ bzImage... 62 secs, modules... 105 secs, done ] (warns: 6)
allmod32: # 8, 8e70122d, Tue_May_12_09_47_56_CEST_2015: 44 kernels/hour, [ bzImage... 58 secs, modules... 103 secs, done ] (warns: 12)
the 'warns: 16' for example means that there are 16 warnings in that
build. If a new warning is emitted by any of the trees I maintain,
then I get a delta like this:
def64: # 1, 5e5f191d, Tue_May_12_08_55_14_CEST_2015: 0 kernels/hour, [ bzImage... 32 secs, modules... 5 secs, done ] (warns: 0)
def32: # 2, ed602bbb, Tue_May_12_08_55_39_CEST_2015: 138 kernels/hour, [ bzImage... 30 secs, modules... 3 secs, done ] (warns: 0)
allno64: # 3, ed602bbb, Tue_May_12_08_56_17_CEST_2015: 112 kernels/hour, [ bzImage... 22 secs, modules... 0 secs, done ] (warns: 2)
allno32: # 4, ed602bbb, Tue_May_12_08_56_51_CEST_2015: 110 kernels/hour, [ bzImage... 21 secs, modules... 0 secs, done ] (warns: 1)
allyes64: # 5, ed602bbb, Tue_May_12_08_57_14_CEST_2015: 119 kernels/hour, [ bzImage... 199 secs, modules... 9 secs, done ] (warns: 11, delta: 1)
allyes32: # 6, ed602bbb, Tue_May_12_08_57_37_CEST_2015: 125 kernels/hour, [ bzImage... 205 secs, modules... 9 secs, done ] (warns: 17, delta: 1)
allmod64: # 7, ed602bbb, Tue_May_12_09_01_06_CEST_2015: 61 kernels/hour, [ bzImage... 60 secs, modules... 103 secs, done ] (warns: 7, delta: 1)
allmod32: # 8, ed602bbb, Tue_May_12_09_04_41_CEST_2015: 44 kernels/hour, [ bzImage... 59 secs, modules... 105 secs, done ] (warns: 13, delta: 1)
(nicely color highligted so I cannot miss it and such.)
Btw., just some feedback, 'random' kernel configs still generate a ton
of warnings:
randconf: # 9, ed602bbb, Tue_May_12_09_07_25_CEST_2015: 39 kernels/hour, [ bzImage... 81 secs, modules... 21 secs, done ] (warns: 6)
randconf: # 10, ed602bbb, Tue_May_12_09_10_10_CEST_2015: 36 kernels/hour, [ bzImage... 43 secs, modules... 20 secs, done ] (warns: 12)
randconf: # 11, ed602bbb, Tue_May_12_09_11_52_CEST_2015: 36 kernels/hour, [ bzImage... 71 secs, modules... 0 secs, done ] (warns: 5)
randconf: # 12, ed602bbb, Tue_May_12_09_12_56_CEST_2015: 37 kernels/hour, [ bzImage... 64 secs, modules... 28 secs, done ] (warns: 16)
randconf: # 13, ed602bbb, Tue_May_12_09_14_07_CEST_2015: 38 kernels/hour, [ bzImage... 106 secs, modules... 0 secs, done ] (warns: 157)
randconf: # 14, ed602bbb, Tue_May_12_09_15_40_CEST_2015: 38 kernels/hour, [ bzImage... 100 secs, modules... 0 secs, done ] (warns: 11)
randconf: # 15, ed602bbb, Tue_May_12_09_17_27_CEST_2015: 37 kernels/hour, [ bzImage... 65 secs, modules... 28 secs, done ] (warns: 26)
randconf: # 16, ed602bbb, Tue_May_12_09_19_08_CEST_2015: 37 kernels/hour, [ bzImage... 70 secs, modules... 0 secs, done ] (warns: 228)
randconf: # 17, ed602bbb, Tue_May_12_09_20_42_CEST_2015: 37 kernels/hour, [ bzImage... 79 secs, modules... 0 secs, done ] (warns: 101)
randconf: # 18, ed602bbb, Tue_May_12_09_21_53_CEST_2015: 38 kernels/hour, [ bzImage... 55 secs, modules... 0 secs, done ] (warns: 6)
randconf: # 19, ed602bbb, Tue_May_12_09_23_13_CEST_2015: 38 kernels/hour, [ bzImage... 49 secs, modules... 0 secs, done ] (warns: 4)
randconf: # 20, ed602bbb, Tue_May_12_09_24_08_CEST_2015: 39 kernels/hour, [ bzImage... 43 secs, modules... B 20 secs, done ] (warns: 28)
randconf: # 21, ed602bbb, Tue_May_12_09_24_58_CEST_2015: 40 kernels/hour, [ bzImage... B 35 secs, modules... 0 secs, done ] (warns: 14)
randconf: # 22, ed602bbb, Tue_May_12_09_26_02_CEST_2015: 40 kernels/hour, [ bzImage... 58 secs, modules... 0 secs, done ] (warns: 16)
randconf: # 23, ed602bbb, Tue_May_12_09_26_38_CEST_2015: 42 kernels/hour, [ bzImage... 48 secs, modules... B 23 secs, done ] (warns: 35)
randconf: # 24, ed602bbb, Tue_May_12_09_27_37_CEST_2015: 42 kernels/hour, [ bzImage... 40 secs, modules... B 15 secs, done ] (warns: 25)
randconf: # 25, ed602bbb, Tue_May_12_09_28_49_CEST_2015: 42 kernels/hour, [ bzImage... 64 secs, modules... 19 secs, done ] (warns: 3)
randconf: # 26, ed602bbb, Tue_May_12_09_29_45_CEST_2015: 43 kernels/hour, [ bzImage... 87 secs, modules... 0 secs, done ] (warns: 15)
randconf: # 27, ed602bbb, Tue_May_12_09_31_09_CEST_2015: 43 kernels/hour, [ bzImage... 87 secs, modules... 0 secs, done ] (warns: 7)
(and 'B' marks known upstream build breakages.)
There are a few hotspots.
But in any case, your policy to police allmodconfig builds is a good
starting point and has a positive effect.
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].
The "let's see what happens" in the changelog referred to the
possibility that an older GCC version that what I use (4.9.2) emits so
much crap that amounts to a regression and that I'll have to zap the
commit. It did not refer to any random enabling of compiler warnings.
I absolutely failed at pointing out all this background work in the
changelog though. Do you want me to rebase it to fix the changelog?
> [...] Right now I get something like six warnings, all from old
> drivers that so dubious things, but that u can live with.
So the current warnings count is 0/0/2/1/10/16/6/12 on the main
configs on your tree.
I also have a separate build system that uses GCC 5.0.1, there the
warnings count is substantially higher:
def64: # 1, 5e5f191d, Tue_May_12_09_56_23_CEST_2015: 0 kernels/hour, [ bzImage... 47 secs, modules... 3 secs, done ] (warns: 4, delta: 4)
def32: # 2, 1e29025d, Tue_May_12_09_56_53_CEST_2015: 116 kernels/hour, [ bzImage... 46 secs, modules... 2 secs, done ] (warns: 4, delta: 4)
allno64: # 3, 1e29025d, Tue_May_12_09_57_44_CEST_2015: 87 kernels/hour, [ bzImage... 24 secs, modules... 0 secs, done ] (warns: 4, delta: 2)
allno32: # 4, 1e29025d, Tue_May_12_09_58_33_CEST_2015: 82 kernels/hour, [ bzImage... 24 secs, modules... 0 secs, done ] (warns: 3, delta: 2)
allyes64: # 5, 1e29025d, Tue_May_12_09_58_58_CEST_2015: 92 kernels/hour, [ bzImage... 386 secs, modules... 9 secs, done ] (warns: 31, delta: 21)
allyes32: # 6, 1e29025d, Tue_May_12_09_59_23_CEST_2015: 99 kernels/hour, [ bzImage... 344 secs, modules... 10 secs, done ] (warns: 36, delta: 20)
allmod64: # 7, 1e29025d, Tue_May_12_10_05_59_CEST_2015: 37 kernels/hour, [ bzImage... 82 secs, modules... 250 secs, done ] (warns: 27, delta: 21)
allmod32: # 8, 1e29025d, Tue_May_12_10_11_54_CEST_2015: 27 kernels/hour, [ bzImage... 75 secs, modules... 243 secs, done ] (warns: 32, delta: 20)
Here's the full list of warnings for allmod64:
make bzImage:
include/linux/blkdev.h:624:26: warning: switch condition has boolean value [-Wswitch-bool]
./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
make modules:
drivers/ata/pata_hpt366.c:376:9: warning: assignment discards ?const? qualifier from pointer target type [-Wdiscarded-array-qualifiers]
drivers/ata/pata_hpt366.c:379:9: warning: assignment discards ?const? qualifier from pointer target type [-Wdiscarded-array-qualifiers]
drivers/ata/pata_hpt366.c:382:9: warning: assignment discards ?const? qualifier from pointer target type [-Wdiscarded-array-qualifiers]
drivers/hid/hid-input.c:1160:67: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
drivers/md/md.c:6375:26: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
drivers/gpu/drm/gma500/cdv_intel_dp.c:869:2: warning: ?i2c_dp_aux_add_bus? is deprecated [-Wdeprecated-declarations]
drivers/mmc/host/sh_mmcif.c:401:4: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
drivers/mmc/host/sh_mmcif.c:402:4: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
drivers/isdn/hardware/eicon/message.c:6115:1: warning: the frame size of 2352 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/scsi/advansys.c:71:2: warning: #warning this driver is still not properly converted to the DMA API [-Wcpp]
drivers/scsi/be2iscsi/be_main.c:3168:18: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
sound/pci/oxygen/oxygen_mixer.c:91:43: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
drivers/usb/renesas_usbhs/common.c:492:25: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
drivers/media/platform/s3c-camif/camif-capture.c:118:10: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
drivers/media/platform/s3c-camif/camif-capture.c:134:10: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
drivers/net/wireless/brcm80211/brcmsmac/phy/phy_n.c:16065:1: warning: the frame size of 3200 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/media/usb/cx231xx/cx231xx-cards.c:1110:1: warning: the frame size of 2064 bytes is larger than 2048 bytes [-Wframe-larger-than=]
drivers/net/wireless/brcm80211/brcmsmac/phy/phy_n.c:17138:1: warning: the frame size of 2768 bytes is larger than 2048 bytes [-Wframe-larger-than=]
No new warnings were triggered by -Wno-sign-compare.
Six of the new warnings are generated due to -Wlogical-not-parentheses,
which looks like false positives in the cases of:
drivers/hid/hid-input.c:1160
drivers/md/md.c:6375
but it's actually pointing out what appears to be a real bug here:
drivers/scsi/be2iscsi/be_main.c:3168
and I'm unsure about:
sound/pci/oxygen/oxygen_mixer.c:91
but it sure looks weird. These are probably correct but are
looking weird:
drivers/media/platform/s3c-camif/camif-capture.c:118
drivers/media/platform/s3c-camif/camif-capture.c:134
and any kernel code that should be routine but makes reviewers
looks twice probably needs improving.
In any case, yes, I fully agree with your point and general policy
that by default we cannot trust GCC's warnings, so I have tested this
change as much as I could.
> Does this add to that? Because if it does, I'm not pulling the end
> result.
Yeah, absolutely. I'll even zap the commit if it generates noise for
older but still widely used GCC versions.
Thanks,
Ingo
* Ingo Molnar <[email protected]> wrote:
> Here's the full list of warnings for allmod64:
>
> make bzImage:
>
> include/linux/blkdev.h:624:26: warning: switch condition has boolean value [-Wswitch-bool]
> ./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
> ./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
> ./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
> ./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
> ./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
> ./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
> ./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
> ./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
And that comes from tip:locking/core, but those warnings do not show
up with GCC 4.9.2: so it's either a GCC 5.0.1 bug, or we missed
something with the WIP queued pv spinlocks changes that newer GCC is
able to notice.
Thanks,
Ingo
On 05/12/2015 04:53 AM, Ingo Molnar wrote:
> * Ingo Molnar<[email protected]> wrote:
>
>> Here's the full list of warnings for allmod64:
>>
>> make bzImage:
>>
>> include/linux/blkdev.h:624:26: warning: switch condition has boolean value [-Wswitch-bool]
>> ./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
>> ./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
>> ./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
>> ./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
>> ./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
>> ./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
>> ./arch/x86/include/asm/qspinlock.h:28:2: warning: implicit declaration of function ?pv_queued_spin_lock_slowpath? [-Wimplicit-function-declaration]
>> ./arch/x86/include/asm/qspinlock.h:33:2: warning: implicit declaration of function ?pv_queued_spin_unlock? [-Wimplicit-function-declaration]
> And that comes from tip:locking/core, but those warnings do not show
> up with GCC 4.9.2: so it's either a GCC 5.0.1 bug, or we missed
> something with the WIP queued pv spinlocks changes that newer GCC is
> able to notice.
>
> Thanks,
>
> Ingo
The only way this can happen is when __ASSEMBLY__ is defined and
asm/qspinlock.h is included. Could you apply the following patch to see
if it can fix the compilation warnings? I don't have a gcc 5 compiler on
hand to verify that.
Cheers,
Longman
---
arch/x86/include/asm/qspinlock.h | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/arch/x86/include/asm/qspinlock.h
b/arch/x86/include/asm/qspinlock.h
index 9d51fae..3ea1c57 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -17,7 +17,10 @@ static inline void native_queued_spin_unlock(struct
qspinlock *lock)
smp_store_release((u8 *)lock, 0);
}
-#ifdef CONFIG_PARAVIRT_SPINLOCKS
+/*
+ * Disable the PV code for assembly to prevent compilation warnings.
+ */
+#if defined(CONFIG_PARAVIRT_SPINLOCKS) && !defined(__ASSEMBLY__)
extern void native_queued_spin_lock_slowpath(struct qspinlock *lock,
u32 val);
extern void __pv_init_lock_hash(void);
extern void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32
val);
--
On Tue, 12 May 2015 10:44:15 +0200, Ingo Molnar said:
> Btw., just some feedback, 'random' kernel configs still generate a ton
> of warnings:
>
> randconf: # 9, ed602bbb, Tue_May_12_09_07_25_CEST_2015: 39 kernels/hour,
[ bzImage... 81 secs, modules... 21 secs, done ] (warns: 6)
> randconf: # 10, ed602bbb, Tue_May_12_09_10_10_CEST_2015: 36 kernels/hour,
[ bzImage... 43 secs, modules... 20 secs, done ] (warns: 12)
> randconf: # 11, ed602bbb, Tue_May_12_09_11_52_CEST_2015: 36 kernels/hour,
[ bzImage... 71 secs, modules... 0 secs, done ] (warns: 5)
> randconf: # 12, ed602bbb, Tue_May_12_09_12_56_CEST_2015: 37 kernels/hour,
[ bzImage... 64 secs, modules... 28 secs, done ] (warns: 16)
> randconf: # 13, ed602bbb, Tue_May_12_09_14_07_CEST_2015: 38 kernels/hour,
[ bzImage... 106 secs, modules... 0 secs, done ] (warns: 157)
> randconf: # 14, ed602bbb, Tue_May_12_09_15_40_CEST_2015: 38 kernels/hour,
[ bzImage... 100 secs, modules... 0 secs, done ] (warns: 11)
> randconf: # 15, ed602bbb, Tue_May_12_09_17_27_CEST_2015: 37 kernels/hour,
[ bzImage... 65 secs, modules... 28 secs, done ] (warns: 26)
> randconf: # 16, ed602bbb, Tue_May_12_09_19_08_CEST_2015: 37 kernels/hour,
[ bzImage... 70 secs, modules... 0 secs, done ] (warns: 228)
> 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
% 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*...
CC init/main.o
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from ./include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from init/main.c:14:
include/asm-generic/qrwlock.h: In function 'queue_write_trylock':
include/asm-generic/qrwlock.h:93:35: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
cnts, cnts | _QW_LOCKED) == cnts);
^
include/linux/compiler.h:132:43: note: in definition of macro 'likely'
# define likely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 1))
^
include/asm-generic/qrwlock.h:93:35: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
cnts, cnts | _QW_LOCKED) == cnts);
^
include/linux/compiler.h:108:47: note: in definition of macro 'likely_notrace'
#define likely_notrace(x) __builtin_expect(!!(x), 1)
^
include/linux/compiler.h:132:56: note: in expansion of macro '__branch_check__'
# define likely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 1))
^
include/asm-generic/qrwlock.h:92:9: note: in expansion of macro 'likely'
return likely(atomic_cmpxchg(&lock->cnts,
^
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from ./include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from init/main.c:14:
./arch/x86/include/asm/uaccess.h: In function 'copy_from_user':
./arch/x86/include/asm/uaccess.h:712:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (likely(sz < 0 || sz >= n))
^
include/linux/compiler.h:132:43: note: in definition of macro 'likely'
# define likely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 1))
^
./arch/x86/include/asm/uaccess.h:712:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (likely(sz < 0 || sz >= n))
^
include/linux/compiler.h:132:51: note: in definition of macro 'likely'
# define likely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 1))
^
./arch/x86/include/asm/uaccess.h:712:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (likely(sz < 0 || sz >= n))
^
include/linux/compiler.h:108:47: note: in definition of macro 'likely_notrace'
#define likely_notrace(x) __builtin_expect(!!(x), 1)
^
include/linux/compiler.h:132:56: note: in expansion of macro '__branch_check__'
# define likely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 1))
^
./arch/x86/include/asm/uaccess.h:712:6: note: in expansion of macro 'likely'
if (likely(sz < 0 || sz >= n))
^
./arch/x86/include/asm/uaccess.h: In function 'copy_to_user':
./arch/x86/include/asm/uaccess.h:730:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (likely(sz < 0 || sz >= n))
^
include/linux/compiler.h:132:43: note: in definition of macro 'likely'
# define likely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 1))
^
./arch/x86/include/asm/uaccess.h:730:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (likely(sz < 0 || sz >= n))
^
include/linux/compiler.h:132:51: note: in definition of macro 'likely'
# define likely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 1))
^
./arch/x86/include/asm/uaccess.h:730:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (likely(sz < 0 || sz >= n))
^
include/linux/compiler.h:108:47: note: in definition of macro 'likely_notrace'
#define likely_notrace(x) __builtin_expect(!!(x), 1)
^
include/linux/compiler.h:132:56: note: in expansion of macro '__branch_check__'
# define likely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 1))
^
./arch/x86/include/asm/uaccess.h:730:6: note: in expansion of macro 'likely'
if (likely(sz < 0 || sz >= n))
^
In file included from ./arch/x86/include/asm/preempt.h:5:0,
from include/linux/preempt.h:18,
from include/linux/spinlock.h:50,
from include/linux/seqlock.h:35,
from include/linux/time.h:5,
from include/linux/stat.h:18,
from include/linux/module.h:10,
from init/main.c:15:
include/linux/percpu-refcount.h: In function 'percpu_ref_get_many':
./arch/x86/include/asm/percpu.h:130:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
((val) == 1 || (val) == -1)) ? \
^
./arch/x86/include/asm/percpu.h:419:34: note: in expansion of macro 'percpu_add_op'
#define this_cpu_add_1(pcp, val) percpu_add_op((pcp), val)
^
include/linux/percpu-defs.h:364:11: note: in expansion of macro 'this_cpu_add_1'
case 1: stem##1(variable, __VA_ARGS__);break; \
^
include/linux/percpu-defs.h:498:33: note: in expansion of macro '__pcpu_size_call'
#define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
^
include/linux/percpu-refcount.h:177:3: note: in expansion of macro 'this_cpu_add'
this_cpu_add(*percpu_count, nr);
^
./arch/x86/include/asm/percpu.h:130:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
((val) == 1 || (val) == -1)) ? \
^
./arch/x86/include/asm/percpu.h:420:34: note: in expansion of macro 'percpu_add_op'
#define this_cpu_add_2(pcp, val) percpu_add_op((pcp), val)
^
include/linux/percpu-defs.h:365:11: note: in expansion of macro 'this_cpu_add_2'
case 2: stem##2(variable, __VA_ARGS__);break; \
^
include/linux/percpu-defs.h:498:33: note: in expansion of macro '__pcpu_size_call'
#define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
^
include/linux/percpu-refcount.h:177:3: note: in expansion of macro 'this_cpu_add'
this_cpu_add(*percpu_count, nr);
^
./arch/x86/include/asm/percpu.h:130:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
((val) == 1 || (val) == -1)) ? \
^
./arch/x86/include/asm/percpu.h:421:34: note: in expansion of macro 'percpu_add_op'
#define this_cpu_add_4(pcp, val) percpu_add_op((pcp), val)
^
include/linux/percpu-defs.h:366:11: note: in expansion of macro 'this_cpu_add_4'
case 4: stem##4(variable, __VA_ARGS__);break; \
^
include/linux/percpu-defs.h:498:33: note: in expansion of macro '__pcpu_size_call'
#define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
^
include/linux/percpu-refcount.h:177:3: note: in expansion of macro 'this_cpu_add'
this_cpu_add(*percpu_count, nr);
^
./arch/x86/include/asm/percpu.h:130:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
((val) == 1 || (val) == -1)) ? \
^
./arch/x86/include/asm/percpu.h:478:35: note: in expansion of macro 'percpu_add_op'
#define this_cpu_add_8(pcp, val) percpu_add_op((pcp), val)
^
include/linux/percpu-defs.h:367:11: note: in expansion of macro 'this_cpu_add_8'
case 8: stem##8(variable, __VA_ARGS__);break; \
^
include/linux/percpu-defs.h:498:33: note: in expansion of macro '__pcpu_size_call'
#define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
^
include/linux/percpu-refcount.h:177:3: note: in expansion of macro 'this_cpu_add'
this_cpu_add(*percpu_count, nr);
^
include/linux/percpu-refcount.h: In function 'percpu_ref_put_many':
./arch/x86/include/asm/percpu.h:130:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
((val) == 1 || (val) == -1)) ? \
^
./arch/x86/include/asm/percpu.h:419:34: note: in expansion of macro 'percpu_add_op'
#define this_cpu_add_1(pcp, val) percpu_add_op((pcp), val)
^
include/linux/percpu-defs.h:364:11: note: in expansion of macro 'this_cpu_add_1'
case 1: stem##1(variable, __VA_ARGS__);break; \
^
include/linux/percpu-defs.h:498:33: note: in expansion of macro '__pcpu_size_call'
#define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
^
include/linux/percpu-defs.h:508:33: note: in expansion of macro 'this_cpu_add'
#define this_cpu_sub(pcp, val) this_cpu_add(pcp, -(typeof(pcp))(val))
^
include/linux/percpu-refcount.h:276:3: note: in expansion of macro 'this_cpu_sub'
this_cpu_sub(*percpu_count, nr);
^
./arch/x86/include/asm/percpu.h:130:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
((val) == 1 || (val) == -1)) ? \
^
./arch/x86/include/asm/percpu.h:420:34: note: in expansion of macro 'percpu_add_op'
#define this_cpu_add_2(pcp, val) percpu_add_op((pcp), val)
^
include/linux/percpu-defs.h:365:11: note: in expansion of macro 'this_cpu_add_2'
case 2: stem##2(variable, __VA_ARGS__);break; \
^
include/linux/percpu-defs.h:498:33: note: in expansion of macro '__pcpu_size_call'
#define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
^
include/linux/percpu-defs.h:508:33: note: in expansion of macro 'this_cpu_add'
#define this_cpu_sub(pcp, val) this_cpu_add(pcp, -(typeof(pcp))(val))
^
include/linux/percpu-refcount.h:276:3: note: in expansion of macro 'this_cpu_sub'
this_cpu_sub(*percpu_count, nr);
^
./arch/x86/include/asm/percpu.h:130:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
((val) == 1 || (val) == -1)) ? \
^
./arch/x86/include/asm/percpu.h:421:34: note: in expansion of macro 'percpu_add_op'
#define this_cpu_add_4(pcp, val) percpu_add_op((pcp), val)
^
include/linux/percpu-defs.h:366:11: note: in expansion of macro 'this_cpu_add_4'
case 4: stem##4(variable, __VA_ARGS__);break; \
^
include/linux/percpu-defs.h:498:33: note: in expansion of macro '__pcpu_size_call'
#define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
^
include/linux/percpu-defs.h:508:33: note: in expansion of macro 'this_cpu_add'
#define this_cpu_sub(pcp, val) this_cpu_add(pcp, -(typeof(pcp))(val))
^
include/linux/percpu-refcount.h:276:3: note: in expansion of macro 'this_cpu_sub'
this_cpu_sub(*percpu_count, nr);
^
./arch/x86/include/asm/percpu.h:130:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
((val) == 1 || (val) == -1)) ? \
^
./arch/x86/include/asm/percpu.h:478:35: note: in expansion of macro 'percpu_add_op'
#define this_cpu_add_8(pcp, val) percpu_add_op((pcp), val)
^
include/linux/percpu-defs.h:367:11: note: in expansion of macro 'this_cpu_add_8'
case 8: stem##8(variable, __VA_ARGS__);break; \
^
include/linux/percpu-defs.h:498:33: note: in expansion of macro '__pcpu_size_call'
#define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
^
include/linux/percpu-defs.h:508:33: note: in expansion of macro 'this_cpu_add'
#define this_cpu_sub(pcp, val) this_cpu_add(pcp, -(typeof(pcp))(val))
^
include/linux/percpu-refcount.h:276:3: note: in expansion of macro 'this_cpu_sub'
this_cpu_sub(*percpu_count, nr);
^
In file included from include/linux/blkdev.h:18:0,
from init/main.c:75:
include/linux/bio.h: In function 'bio_next_split':
include/linux/bio.h:388:14: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (sectors >= bio_sectors(bio))
^
init/main.c: In function 'do_initcalls':
init/main.c:860:24: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++)
^
* [email protected] <[email protected]> wrote:
> On Tue, 12 May 2015 10:44:15 +0200, Ingo Molnar said:
>
> > Btw., just some feedback, 'random' kernel configs still generate a ton
> > of warnings:
> >
> > randconf: # 9, ed602bbb, Tue_May_12_09_07_25_CEST_2015: 39 kernels/hour,
> [ bzImage... 81 secs, modules... 21 secs, done ] (warns: 6)
> > randconf: # 10, ed602bbb, Tue_May_12_09_10_10_CEST_2015: 36 kernels/hour,
> [ bzImage... 43 secs, modules... 20 secs, done ] (warns: 12)
> > randconf: # 11, ed602bbb, Tue_May_12_09_11_52_CEST_2015: 36 kernels/hour,
> [ bzImage... 71 secs, modules... 0 secs, done ] (warns: 5)
> > randconf: # 12, ed602bbb, Tue_May_12_09_12_56_CEST_2015: 37 kernels/hour,
> [ bzImage... 64 secs, modules... 28 secs, done ] (warns: 16)
> > randconf: # 13, ed602bbb, Tue_May_12_09_14_07_CEST_2015: 38 kernels/hour,
> [ bzImage... 106 secs, modules... 0 secs, done ] (warns: 157)
> > randconf: # 14, ed602bbb, Tue_May_12_09_15_40_CEST_2015: 38 kernels/hour,
> [ bzImage... 100 secs, modules... 0 secs, done ] (warns: 11)
> > randconf: # 15, ed602bbb, Tue_May_12_09_17_27_CEST_2015: 37 kernels/hour,
> [ bzImage... 65 secs, modules... 28 secs, done ] (warns: 26)
> > randconf: # 16, ed602bbb, Tue_May_12_09_19_08_CEST_2015: 37 kernels/hour,
> [ bzImage... 70 secs, modules... 0 secs, done ] (warns: 228)
>
> > 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
>
> % 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*...
That's a prerelease of GCC, right? So I think GCC gets constant
propagation from various builtins wrong or so.
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.
So it does not seem to have much utility, and seems to be horribly
broken on fresh GCC.
I'll remove the commit.
Thanks,
Ingo
On May 13, 2015, at 4:22 AM, Ingo Molnar <[email protected]> wrote:
> * [email protected] <[email protected]> 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-
On Wed, May 13, 2015 at 10:33 AM, Louis Langholtz <[email protected]> wrote:
>
> Are warnings about sign-comparisons an architecture specific issue?
No, I think it's a combination of
(a) some architectures don't end up caring too much about warnings (I
guess you could call this an "architecture specific issue")
(b) the real problems tend to be in gcc versions that do f*cking
insane things and warn for code that cannot possibly be written better
any other way. Some architectures use a very constrained set of
compilers, and their set may not have the particular problem.
The classic example of (b) is the whole "comparison with a constant".
Gcc has gotten this wrong so many times that it's not even funny.
The fact is, we often use constants with the "wrong" sign. Tough. Part
of it is that signed constants are simply the only sane case, even if
you then compare against unsigned variables. A compiler that complains
about that is a shit compiler. It's that simple.
This is why "-Wno-sign-compare" was added in the first place. People
who think that you should always compare unsigned variables against
unsigned constants are simply wrong.
And yes, we have a lot of them inside macros. We often use macros as a
way to do generics in C, so code like this:
#define percpu_add_op(var, val) \
do { \
...
const int pao_ID__ = (__builtin_constant_p(val) && \
((val) == 1 || (val) == -1)) ? \
where we do magic things if "val" is 1 or -1 (generating "inc" and
"dec" respectively) is not insane. But because this is a generic
thing, sometimes we pass unsigned things around, and you get that
"compare 'unsigned' val against '-1'" thing. Tough.
Sure, we can add random casts in these things. In this particular
example, we could do things like cast both (val) and -1 to
"typeof(var)" and it wouldn't be "wrong". After all, it's really just
meant for a heuristic (in this case it's a "let's see if this case of
addition is a trivial special case of adding -1, and we'll turn it
into a smaller instruction").
So those kinds of cast things don't actually improve the code, and in
many cases they actively make it less obvious. They aren't worth it.
So it's a tradeoff. Which one is better: get rid of stupid warnings by
using "-Wno-sign-compare" or by polluting the source code with
pointless type casts?
If -Wno-sign-compare had higher quality, I'd be perfectly willing to
add a few pointless casts. But the problem with the gcc sign-compare
warning is that it has traditionally (and apparenrly _still_) had the
quality of undiluted horse-shit.
Which makes it an easy decision to make: "-Wno-sign-compare" is the
right solution. Shut up the crap warnings, without making the source
worse.
Linus