Message ID | 20220319222228.4160598-1-morbo@google.com |
---|---|
State | New |
Headers | show |
Series | [v2] gpiolib: acpi: use correct format characters | expand |
So I think that clang warning is only annoying, not helpful, but: On Sat, Mar 19, 2022 at 3:22 PM Bill Wendling <morbo@google.com> wrote: > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > index a5495ad31c9c..92dd9b8784f2 100644 > --- a/drivers/gpio/gpiolib-acpi.c > +++ b/drivers/gpio/gpiolib-acpi.c > @@ -388,9 +388,9 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares, > > if (pin <= 255) { > char ev_name[5]; > - sprintf(ev_name, "_%c%02hhX", > + sprintf(ev_name, "_%c%02X", This part I approve of. > agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L', > - pin); > + (unsigned char)pin); But this cast seems pointless and wrong. Casts in general are bad, and should be avoided unless there's a real reason for them. And that reason doesn't seem to exist. We don't actually want to truncate the value of 'pin', and just a few lines earlier actually checked that it is in range. And if 'pin' can't be negative - it comes from a 'u16' table dereference - but even if it could have been that would have been a different bug here anyway (and should have been fixed by tightening the check). So the cast doesn't add anything - not for humans, and not for a compiler that could just optimize it away because it saw the range check. End result: just fix the pointless 'hh' in the print specifier. It doesn't add anything, and only causes problems. Anybody who uses '%02X to print a byte should only use it for byte values - and the code already does. Of course, the _reason_ for this all was a warning that was pointless to begin with, and should never have existed. Clang was not smart enough to take the range knowledge that it _could_ have taken into account, and instead wrote out a completely bogus warning. It's completely bogus not just because clang didn't do a sufficiently good job of range analysis - it's completely bogus because a 'varargs' function DOES NOT TAKE arguments of type 'char'. So the *only* reason to use '%hhX' in the first place is that you *want* the sprintf() to actually limit the value to a byte for you (possibly because you have a signed char, know it will sign-extend to 'int', and want to limit it back to 8 bits). If you *actually* had a 'unsigned char' to begin with, you'd be completely insane to use %hhX. It's just pointless. So warning that '%hhX' is paired with an 'int' is all just completely mindless and wrong. Linus
On Sat, Mar 19, 2022 at 3:54 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > So warning that '%hhX' is paired with an 'int' is all just completely > mindless and wrong. Sadly, I can see a different bogus warning reason why people would want to use '%02hhX'. Again, the *sane* thing from a human perspective is to use '%02X. But if the compiler doesn't do any range analysis at all, it could decide that "Oh, that print format could need up to 8 bytes of space in the result". Using '%02hhX' would cut that down to two. And since we use char ev_name[5]; and currently use "_%c%02hhX" as the format string, even a compiler that doesn't notice that "pin <= 255" test that guards this all will go "ok, that's at most 4 bytes and the final NUL termination, so it's fine". While a compiler - like gcc - that only sees that the original source of the 'pin' value is a 'unsigned short' array, and then doesn't take the "pin <= 255" into account, will warn like this: drivers/gpio/gpiolib-acpi.c: In function 'acpi_gpiochip_request_interrupt': drivers/gpio/gpiolib-acpi.c:206:24: warning: '%02X' directive writing between 2 and 4 bytes into a region of size 3 [-Wformat-overflow=] sprintf(ev_name, "_%c%02X", ^~~~ drivers/gpio/gpiolib-acpi.c:206:20: note: directive argument in the range [0, 65535] because gcc isn't being very good at that argument range analysis either. In other words, the original use of 'hhx' was bogus to begin with, and due to *another* compiler warning being bad, and we had that bad code being written back in 2016 to work around _that_ compiler warning (commit e40a3ae1f794: "gpio: acpi: work around false-positive -Wstring-overflow warning"). Sadly, two different bad compiler warnings together does not make for one good one. It just makes for even more pain. End result: I think the simplest and cleanest option is simply this: --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -387,8 +387,8 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares, pin = agpio->pin_table[0]; if (pin <= 255) { - char ev_name[5]; - sprintf(ev_name, "_%c%02hhX", + char ev_name[8]; + sprintf(ev_name, "_%c%02X", agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L', pin); if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name, &evt_handle))) which undoes that '%hhX' change for gcc, and replaces it with just using a slightly bigger stack allocation. It's not like a 5-byte allocation is in any way likely to have saved any actual stack, since all the other variables in that function are 'int' or bigger. False-positive compiler warnings really do make people write worse code, and that's a problem. But on a scale of bad code, I feel that extending the buffer trivially is better than adding a pointless cast that literally makes no sense. At least in this case the end result isn't unreadable or buggy. We've had several cases of bad compiler warnings that caused changes that were actually horrendously wrong. Linus
On Sat, Mar 19, 2022 at 3:55 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > So I think that clang warning is only annoying, not helpful, but: > > On Sat, Mar 19, 2022 at 3:22 PM Bill Wendling <morbo@google.com> wrote: > > > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > > index a5495ad31c9c..92dd9b8784f2 100644 > > --- a/drivers/gpio/gpiolib-acpi.c > > +++ b/drivers/gpio/gpiolib-acpi.c > > @@ -388,9 +388,9 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares, > > > > if (pin <= 255) { > > char ev_name[5]; > > - sprintf(ev_name, "_%c%02hhX", > > + sprintf(ev_name, "_%c%02X", > > This part I approve of. > > > agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L', > > - pin); > > + (unsigned char)pin); > > But this cast seems pointless and wrong. > You're right. I was trying to ensure that the patch didn't change behavior. But the cast achieves nothing. Thanks! -bw > Casts in general are bad, and should be avoided unless there's a real > reason for them. And that reason doesn't seem to exist. We don't > actually want to truncate the value of 'pin', and just a few lines > earlier actually checked that it is in range. > > And if 'pin' can't be negative - it comes from a 'u16' table > dereference - but even if it could have been that would have been a > different bug here anyway (and should have been fixed by tightening > the check). > > So the cast doesn't add anything - not for humans, and not for a > compiler that could just optimize it away because it saw the range > check. > > End result: just fix the pointless 'hh' in the print specifier. It > doesn't add anything, and only causes problems. Anybody who uses '%02X > to print a byte should only use it for byte values - and the code > already does. > > Of course, the _reason_ for this all was a warning that was pointless > to begin with, and should never have existed. Clang was not smart > enough to take the range knowledge that it _could_ have taken into > account, and instead wrote out a completely bogus warning. > > It's completely bogus not just because clang didn't do a sufficiently > good job of range analysis - it's completely bogus because a 'varargs' > function DOES NOT TAKE arguments of type 'char'. > > So the *only* reason to use '%hhX' in the first place is that you > *want* the sprintf() to actually limit the value to a byte for you > (possibly because you have a signed char, know it will sign-extend to > 'int', and want to limit it back to 8 bits). > > If you *actually* had a 'unsigned char' to begin with, you'd be > completely insane to use %hhX. It's just pointless. > > So warning that '%hhX' is paired with an 'int' is all just completely > mindless and wrong. > > Linus
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index a5495ad31c9c..92dd9b8784f2 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -388,9 +388,9 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares, if (pin <= 255) { char ev_name[5]; - sprintf(ev_name, "_%c%02hhX", + sprintf(ev_name, "_%c%02X", agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L', - pin); + (unsigned char)pin); if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name, &evt_handle))) handler = acpi_gpio_irq_handler; }
When compiling with -Wformat, clang emits the following warning: drivers/gpio/gpiolib-acpi.c:393:4: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat] pin); ^~~ The types of these arguments are unconditionally defined, so this patch updates the format character to the correct ones casts to unsigned to retain the behavior or the "hh" modifier.. Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling <morbo@google.com> --- v2 - Cast "pin" to retain the same width as the original. --- drivers/gpio/gpiolib-acpi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)