[PATCH] gpiolib: acpi: use correct format characters

[PATCH] gpiolib: acpi: use correct format characters

[Bill Wendling]

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 for ints and unsigned
ints.

Link: ClangBuiltLinux/linux#378
Signed-off-by: Bill Wendling <morbo@google.com>
---
 drivers/gpio/gpiolib-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index a5495ad31c9c..be6fb2ad2c4a 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -388,7 +388,7 @@ 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);
 		if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name, &evt_handle)))
-- 
2.35.1.723.g4982287a31-goog

Re: [PATCH] gpiolib: acpi: use correct format characters

[Andy Shevchenko]

On Wed, Mar 16, 2022 at 02:30:55PM -0700, Bill Wendling wrote:
> 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 for ints and unsigned
> ints.

hhX specifier refers to unsigned char. It's a bug in the compiler.

NAK.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: acpi: use correct format characters

[Andy Shevchenko]

On Thu, Mar 17, 2022 at 11:06:51AM +0200, Andy Shevchenko wrote:
> On Wed, Mar 16, 2022 at 02:30:55PM -0700, Bill Wendling wrote:
> > 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 for ints and unsigned
> > ints.
> 
> hhX specifier refers to unsigned char. It's a bug in the compiler.
> 
> NAK.

Oh, I read this wrong, sorry. The pin has been checked to fit in one byte,
but its type is bigger indeed.

I will apply your patch right away and send as a fix after rc1.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: acpi: use correct format characters

[Nick Desaulniers]

On Thu, Mar 17, 2022 at 2:07 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Mar 16, 2022 at 02:30:55PM -0700, Bill Wendling wrote:
> > 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 for ints and unsigned
> > ints.
>
> hhX specifier refers to unsigned char. It's a bug in the compiler.
>
> NAK.

Andy,
Our goal is to enable -Wformat for CC=clang.  Please see also:
commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
unnecessary %h[xudi] and %hh[xudi]")
and the lore link it cites.
https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
(I saw your follow up; this patch is one of the less controversial
ones though since the types are not ones that are promoted).

Bill,
I just remembered that we will want to explicitly set
-Wno-format-pedantic when enabling -Wformat. Remember that -Wformat is
a group flag that turns on other flags, such as -Wformat-security
(currently disabled) and -Wformat-pedantic.  See also:
https://reviews.llvm.org/rGcc01d6421f4a896820c02da2ea92b82d973b431e
commit a8735821d198 ("Kbuild: Disable the -Wformat-security gcc flag")

It may be helpful to cite
commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
unnecessary %h[xudi] and %hh[xudi]")
in future commits that change the format flags for types that are promoted.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] gpiolib: acpi: use correct format characters

[Andy Shevchenko]

On Thu, Mar 17, 2022 at 11:11:21AM -0700, Nick Desaulniers wrote:
> On Thu, Mar 17, 2022 at 2:07 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Mar 16, 2022 at 02:30:55PM -0700, Bill Wendling wrote:

> > NAK.

I hope you read my follow up to this.

> Andy,
> Our goal is to enable -Wformat for CC=clang.  Please see also:
> commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> unnecessary %h[xudi] and %hh[xudi]")

Not that I agree on that commit for %h[h]x

	signed char ch = -1;
	printf("%x\n", ch);
	printf("%hhx\n", ch);

That's why my first reaction on this change was NAK.

> and the lore link it cites.
> https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
> (I saw your follow up; this patch is one of the less controversial
> ones though since the types are not ones that are promoted).

That said, I hope you are very well aware of the case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: acpi: use correct format characters

[Nick Desaulniers]

On Fri, Mar 18, 2022 at 7:04 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Mar 17, 2022 at 11:11:21AM -0700, Nick Desaulniers wrote:
> > Our goal is to enable -Wformat for CC=clang.  Please see also:
> > commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> > unnecessary %h[xudi] and %hh[xudi]")
>
> Not that I agree on that commit for %h[h]x
>
>         signed char ch = -1;
>         printf("%x\n", ch);
>         printf("%hhx\n", ch);

Will print:
ffffffff
ff

Maybe we should reconsider our recommendations for signed types?

It's probably worth adding `signed char` explicitly to
Documentation/core-api/printk-formats.rst, as it is a distinct type
from `char` in C.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] gpiolib: acpi: use correct format characters

[Bill Wendling]

On Fri, Mar 18, 2022 at 11:01 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Fri, Mar 18, 2022 at 7:04 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Mar 17, 2022 at 11:11:21AM -0700, Nick Desaulniers wrote:
> > > Our goal is to enable -Wformat for CC=clang.  Please see also:
> > > commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> > > unnecessary %h[xudi] and %hh[xudi]")
> >
> > Not that I agree on that commit for %h[h]x
> >
> >         signed char ch = -1;
> >         printf("%x\n", ch);
> >         printf("%hhx\n", ch);
>
> Will print:
> ffffffff
> ff
>
I noticed this. My first thought was to do something akin to:

  printf("%x\n", (u8)ch);

but went the route of removing the "h" qualifiers to be more in line
with previous fixes. I will be happy to change this patch if that's
what you would prefer.

-bw

Re: [PATCH] gpiolib: acpi: use correct format characters

[Nick Desaulniers]

On Fri, Mar 18, 2022 at 11:25 AM Bill Wendling <morbo@google.com> wrote:
>
> On Fri, Mar 18, 2022 at 11:01 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Fri, Mar 18, 2022 at 7:04 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Mar 17, 2022 at 11:11:21AM -0700, Nick Desaulniers wrote:
> > > > Our goal is to enable -Wformat for CC=clang.  Please see also:
> > > > commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> > > > unnecessary %h[xudi] and %hh[xudi]")
> > >
> > > Not that I agree on that commit for %h[h]x
> > >
> > >         signed char ch = -1;
> > >         printf("%x\n", ch);
> > >         printf("%hhx\n", ch);
> >
> > Will print:
> > ffffffff
> > ff
> >
> I noticed this. My first thought was to do something akin to:
>
>   printf("%x\n", (u8)ch);
>
> but went the route of removing the "h" qualifiers to be more in line
> with previous fixes. I will be happy to change this patch if that's
> what you would prefer.

Should we add a note diagnostic to clang suggesting the explicit cast
as one method of silencing the warning?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] gpiolib: acpi: use correct format characters

[Linus Torvalds]

On Fri, Mar 18, 2022 at 11:01 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Maybe we should reconsider our recommendations for signed types?

For 'hhx' is probably does make sense in some cases.

That said, for kernel work, if you work on byte values, I would
seriously suggest not using 'char' at all, which has badly defined
sign.

And 'signed char' makes no sense either.

So while 'hhx' makes sense in the general case, for kernel work I'd
much rather see "don't use stupid types".

So why not just use 'unsigned char' (or 'u8' if you think typing is boring).

                 Linus

Re: [PATCH] gpiolib: acpi: use correct format characters

[Bill Wendling]

On Fri, Mar 18, 2022 at 11:29 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Fri, Mar 18, 2022 at 11:25 AM Bill Wendling <morbo@google.com> wrote:
> >
> > On Fri, Mar 18, 2022 at 11:01 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > >
> > > On Fri, Mar 18, 2022 at 7:04 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Thu, Mar 17, 2022 at 11:11:21AM -0700, Nick Desaulniers wrote:
> > > > > Our goal is to enable -Wformat for CC=clang.  Please see also:
> > > > > commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> > > > > unnecessary %h[xudi] and %hh[xudi]")
> > > >
> > > > Not that I agree on that commit for %h[h]x
> > > >
> > > >         signed char ch = -1;
> > > >         printf("%x\n", ch);
> > > >         printf("%hhx\n", ch);
> > >
> > > Will print:
> > > ffffffff
> > > ff
> > >
> > I noticed this. My first thought was to do something akin to:
> >
> >   printf("%x\n", (u8)ch);
> >
> > but went the route of removing the "h" qualifiers to be more in line
> > with previous fixes. I will be happy to change this patch if that's
> > what you would prefer.
>
> Should we add a note diagnostic to clang suggesting the explicit cast
> as one method of silencing the warning?

I don't think we should offer multiple suggestions in the notes. It
could become confusing and make the diagnostic messages much bigger.
That doesn't mean we couldn't change the suggestion. :-)

-bw

Re: [PATCH] gpiolib: acpi: use correct format characters

[Linus Torvalds]

On Fri, Mar 18, 2022 at 11:29 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Should we add a note diagnostic to clang suggesting the explicit cast
> as one method of silencing the warning?

On the compiler side, I would love to see warnings about the ambiguity
of the sign of 'char' in the general case.

That said, I tried to add that to 'sparse' long long ago, and couldn't
make it work sanely. All the approaches I tried all get _way_ too many
false positives.

I tried to come up with some way of figuring out "this code acts
differently depending on whether 'char' is signed or not" and warning
about it, and never could.

And I suspect the same is true even for the much moire limited case of
only format warnings.

Because it's a *bad* idea to use '%d' (or almost any other format
specifier) together with a 'char' argument, but only if you don't know
the range of the char argument.

But the other side of the argument is that quite often, people *do*
know the range of the 'char' argument. If your 'char' type thing comes
from some array or string that you control, and you used 'char' simply
because you know you have small values (typical example: use it for an
array of booleans etc), then it would be very annoying if the compiler
warned you about using '%d'.

There is no reason to use '%hhd' when you know your data range is [0,1].

So honestly, I don't think you can come up with a sane warning that
doesn't cause *way* too many false positives and just annoys people.

I'd love to be proven wrong. In fact, I'd _really_ love to be proven
wrong for that generic case. The "sometimes 'char' is signed,
sometimes it is unsigned, and it depends on the architecture and the
compiler flags" can be a real problem.

                Linus

Re: [PATCH] gpiolib: acpi: use correct format characters

[Bill Wendling]

On Fri, Mar 18, 2022 at 11:41 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Mar 18, 2022 at 11:29 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Should we add a note diagnostic to clang suggesting the explicit cast
> > as one method of silencing the warning?
>
> On the compiler side, I would love to see warnings about the ambiguity
> of the sign of 'char' in the general case.
>
> That said, I tried to add that to 'sparse' long long ago, and couldn't
> make it work sanely. All the approaches I tried all get _way_ too many
> false positives.
>
> I tried to come up with some way of figuring out "this code acts
> differently depending on whether 'char' is signed or not" and warning
> about it, and never could.
>
> And I suspect the same is true even for the much moire limited case of
> only format warnings.
>
> Because it's a *bad* idea to use '%d' (or almost any other format
> specifier) together with a 'char' argument, but only if you don't know
> the range of the char argument.
>
> But the other side of the argument is that quite often, people *do*
> know the range of the 'char' argument. If your 'char' type thing comes
> from some array or string that you control, and you used 'char' simply
> because you know you have small values (typical example: use it for an
> array of booleans etc), then it would be very annoying if the compiler
> warned you about using '%d'.
>
> There is no reason to use '%hhd' when you know your data range is [0,1].
>
> So honestly, I don't think you can come up with a sane warning that
> doesn't cause *way* too many false positives and just annoys people.
>
> I'd love to be proven wrong. In fact, I'd _really_ love to be proven
> wrong for that generic case. The "sometimes 'char' is signed,
> sometimes it is unsigned, and it depends on the architecture and the
> compiler flags" can be a real problem.
>
My first thought is that this might be better suited for a static
analyzer, like clang-tidy, that can do deeper analysis on code. It
might still be difficult to weed out all of the false positives, but
could be useful for specific offenders.

-bw

[PATCH v2] gpiolib: acpi: use correct format characters

[Bill Wendling]

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(-)

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;
 	}
-- 
2.35.1.894.gb6a874cedc-goog

Re: [PATCH v2] gpiolib: acpi: use correct format characters

[Linus Torvalds]

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

Re: [PATCH v2] gpiolib: acpi: use correct format characters

[Linus Torvalds]

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

Re: [PATCH v2] gpiolib: acpi: use correct format characters

[Andy Shevchenko]

On Sun, Mar 20, 2022 at 1:06 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Mar 19, 2022 at 3:54 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:

...

> End result: I think the simplest and cleanest option is simply this:

Would you sign off on this? I will then replace the original patch
with your version.

And for the record I have a follow up patch to clearly show that pin
is always unsigned, induced by this discussion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpiolib: acpi: use correct format characters

[Linus Torvalds]

On Mon, Mar 21, 2022 at 3:09 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Would you sign off on this? I will then replace the original patch
> with your version.

Sure. Note that I (intentionally) do bogus indentation of my inline
patches, because that thing wasn't actually _tested_. It's obvious
enough and should fix the issue, but I just wanted to point that out.

With that said, it's _so_ obviously correct (famous last words) that
I'll happily add my sign-off to it:

    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

and I suspect the explanations in the email could probably be used as
much of a commit message.

> And for the record I have a follow up patch to clearly show that pin
> is always unsigned, induced by this discussion.

Yeah, that sounds like a good idea - for a compiler it was obvious due
to the load from a 'u16' array, but a human would actually have to go
and check what that pin_table[] array was to see that "yup, that can't
be negative in 'int'".

                Linus

Re: [PATCH v2] gpiolib: acpi: use correct format characters

[Bill Wendling]

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