• Linus Torvalds's avatar
    gpiolib: acpi: use correct format characters · 213d266e
    Linus Torvalds authored
    When compiling with -Wformat, clang emits the following warning:
    
      gpiolib-acpi.c:393:4: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
                            pin);
                            ^~~
    
    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:
    
      gpiolib-acpi.c: In function 'acpi_gpiochip_request_interrupt':
      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",
                                ^~~~
      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 e40a3ae1: "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 the
    proposed change 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.
    
    Fixes: e40a3ae1 ("gpio: acpi: work around false-positive -Wstring-overflow warning")
    Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
    Signed-off-by: default avatarAndy Shevchenko <andriy.shevchenko@linux.intel.com>
    213d266e
gpiolib-acpi.c 41.4 KB