Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
From: Jean Delvare <jdelvare@suse.de>
To: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
	linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hwmon: (smsc47m1) fix outside array bounds warnings
Date: Wed, 22 May 2019 17:08:48 +0200
Message-ID: <20190522170848.198ccea6@endymion> (raw)
In-Reply-To: <20190521044456.26431-1-yamada.masahiro@socionext.com>

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
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.

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.

-- 
Jean Delvare
SUSE L3 Support

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-21  4:44 Masahiro Yamada
2019-05-22 15:08 ` Jean Delvare [this message]
2019-05-28  1:24   ` Guenter Roeck
2019-05-28  3:09     ` Masahiro Yamada
2019-05-28 13:25       ` Guenter Roeck

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190522170848.198ccea6@endymion \
    --to=jdelvare@suse.de \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=yamada.masahiro@socionext.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org linux-hwmon@archiver.kernel.org
	public-inbox-index linux-hwmon

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/ public-inbox