Messages in this thread | | | Subject | Re: [PATCH] hwmon: (smsc47m1) fix outside array bounds warnings | From | Guenter Roeck <> | Date | Tue, 28 May 2019 06:25:20 -0700 |
| |
On 5/27/19 8:09 PM, Masahiro Yamada wrote: > On Tue, May 28, 2019 at 10:25 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 5/22/19 8:08 AM, Jean Delvare wrote: >>> Hi Masahiro, >>> >>> On Tue, 21 May 2019 13:44:56 +0900, Masahiro Yamada wrote: >>>> Kbuild test robot reports outside array bounds warnings: >>>> >>>> CC [M] drivers/hwmon/smsc47m1.o >>>> drivers/hwmon/smsc47m1.c: In function 'fan_div_store': >>>> drivers/hwmon/smsc47m1.c:370:49: warning: array subscript [0, 2] is outside array bounds of 'u8[3]' {aka 'unsigned char[3]'} [-Warray-bounds] >>>> tmp = 192 - (old_div * (192 - data->fan_preload[nr]) >>>> ~~~~~~~~~~~~~~~~~^~~~ >>>> drivers/hwmon/smsc47m1.c:372:19: warning: array subscript [0, 2] is outside array bounds of 'u8[3]' {aka 'unsigned char[3]'} [-Warray-bounds] >>>> data->fan_preload[nr] = clamp_val(tmp, 0, 191); >>>> ~~~~~~~~~~~~~~~~~^~~~ >>>> drivers/hwmon/smsc47m1.c:373:53: warning: array subscript [0, 2] is outside array bounds of 'const u8[3]' {aka 'const unsigned char[3]'} [-Warray-bounds] >>>> smsc47m1_write_value(data, SMSC47M1_REG_FAN_PRELOAD[nr], >>>> ~~~~~~~~~~~~~~~~~~~~~~~~^~~~ >>> >>> These messages are pretty confusing. Subscript [0, 2] would refer to a >>> bi-dimensional array, while these are 1-dimension arrays. If [0, 2] >>> means something else, I still don't get it, because both indexes 0 and >>> 2 are perfectly within bounds of a 3-element array. So what do these >>> messages mean exactly? Looks like a bogus checker to me. >>> >>>> The index field in the SENSOR_DEVICE_ATTR_R* defines is 0, 1, or 2. >>>> However, the compiler never knows the fact that the default in the >>>> switch statement is unreachable. >>>> >>>> Reported-by: kbuild test robot <lkp@intel.com> >>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >>>> --- >>>> >>>> drivers/hwmon/smsc47m1.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/hwmon/smsc47m1.c b/drivers/hwmon/smsc47m1.c >>>> index 5f92eab24c62..e00102e05666 100644 >>>> --- a/drivers/hwmon/smsc47m1.c >>>> +++ b/drivers/hwmon/smsc47m1.c >>>> @@ -364,6 +364,10 @@ static ssize_t fan_div_store(struct device *dev, >>>> tmp |= data->fan_div[2] << 4; >>>> smsc47m1_write_value(data, SMSC47M2_REG_FANDIV3, tmp); >>>> break; >>>> + default: >>>> + WARN_ON(1); >>>> + mutex_unlock(&data->update_lock); >>>> + return -EINVAL; >>>> } >>> >>> So basically the code is fine, the checker (which checker, BTW?) >>> incorrectly thinks it isn't, and you propose to add dead code to make >>> the checker happy? >>> >>> I disagree with this approach. Ideally the checker must be improved to >> >> Me too. I understand and accept that we sometimes initialize variables >> to make he compiler happy, but this goes a bit too far. We really should >> not add dead code - it creates the impression that it can be reached, >> and would live forever for no good reason. >> >>> understand that the code is correct. If that's not possible, we should >>> be allowed to annotate the code to skip that specific check on these >>> specific lines, because it has been inspected by a knowledgeable human >>> and confirmed to be correct. >>> >> Agreed. >> >>> And if that it still not "possible", then the least intrusive fix would > be to make one of the valid cases the default. But adding new code >>> which will never be executed, but must still be compiled and stored, >>> no, thank you. Another code checker could legitimately complain about >>> that actually. >>> >>> IMHO if code checkers return false positives then they are not helping >>> us and should not be used in the first place. >>> >> Checkers are always only providing guidelines and should never be taken >> at face value. >> >> In summary - NACK. >> >> Guenter >> > > What you guys repeatedly called "checker" is GCC 8. > > Intel's 0day bot reported this, and I was also able to reproduce the warnings > by using the kernel.org toolchain available at: > https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/x86_64-gcc-8.1.0-nolibc-sh4-linux.tar.xz > > > I also checked "git log --grep=Warray-bounds", > and I saw people were fixing this kind of warnings. > And, I am really annoyed by the 0day bot. > > That's why I sent this patch > despite I have no interest in this driver. > > > Having said that, I cannot reproduce these warnings > by other compilers than sh4-linux-gcc. > > So, probably these warnings are false positive. > > > > Currently, I have 3 options I can do: > > [1] I will send an alternative patch to > clarify the unreachable path for both compilers and humans > without adding dead code. > > |@@ -351,6 +351,8 @@ static ssize_t fan_div_store(struct device *dev, > | tmp |= data->fan_div[2] << 4; > | smsc47m1_write_value(data, SMSC47M2_REG_FANDIV3, tmp); > | break; > |+ default: > |+ unreachable(); > | } > | > | /* Preserve fan min */ > > [2] I will send your feed-backs to the maintainer of 0day bot, > and persuade him to stop reporting this. > > [3] I will accept that 0day bot will continue sending this report forever. > So, I will configure my mailer so that this report > will immediately go to the trash box. > > > > > As I already said, I have _zero_ interest in this driver. > > Only the problem I have is, I repeatedly receive annoying reports > from 0day bot, where my patch is not the root cause of the warnings at all. > I know, happens all the time to me as well. Worse is that in some cases I don't even know how to fix it (it is not always as easy as it is here).
> Looks like you both believe the current code is OK as-is, > so I am not sure you are happy with [1]. > I am not exactly happy about it, but I would accept it. Just add to the description that this is to make the compiler happy.
Guenter
> I can try [2] first. > > [3] is the last resort for me. > >
| |