Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure
From: Andre Przywara <andre.przywara@arm.com>
Date: 2023-09-05 16:01:43
Also in:
kernel-janitors, linux-phy, linux-sunxi, lkml
On Tue, 5 Sep 2023 13:32:08 +0300 Dan Carpenter [off-list ref] wrote: Hi,
On Mon, Sep 04, 2023 at 12:58:55AM +0100, Andre Przywara wrote:quoted
On Sun, 3 Sep 2023 12:11:06 +0200 Christophe JAILLET [off-list ref] wrote:quoted
With gcc 12.3.0, when this file is built, we get errors such as: drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’: drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=] 790 | snprintf(name, sizeof(name), "usb%d_vbus", i); | ^~~~~ drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16 790 | snprintf(name, sizeof(name), "usb%d_vbus", i); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Because of the possible value of 'i', this can't be an issue in real worldWould using "u8 i;" help? After all currently there are only 4 PHYs max, and in general this isn't expected to be more than a "handful", so 8 bits should be plenty. An unsigned is better anyway.Generally unsigned types are trickier and cause bugs. I've blogged about this before. The title is a probably more negative than it should have been. https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/ My blog mentions u8 i. I would say avoid that unless forced by an API.
Fair enough, the reason I suggested u8 was to allow us using "%u" in the snprintf, so any static checker would not try to account for a potential '-' character. Because not doing so would spoil that approach for the "usb%d_hsic_12M" string further down.
quoted
It leaves a bit of a bitter taste, though, as we shouldn't do this kind type tweaking, especially not to work around the compiler trying to be clever, but then not seeing the whole picture (that "i" is bounded by compile time constants not exceeding "4").Yeah. There is always the option of just ignoring the static checker when it tells you to write bad code.
Agreed on that, though I find those diagnostics useful, and just ignoring or masking them might come back and haunt us later. So I still think we should fix this, one way or the other. But I feel this goes quite far into bikeshedding territory, so we should probably just go with name[32]. Cheers, Andre.
You don't have to even look at the *whole* picture to know that GCC is wrong. The BIT(i) would overflow if i is more than 31. regards, dan carpenter
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel