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 <[email protected]>
---
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
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
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
On Thu, Mar 17, 2022 at 2:07 AM Andy Shevchenko
<[email protected]> 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
On Fri, Mar 18, 2022 at 11:29 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Fri, Mar 18, 2022 at 11:25 AM Bill Wendling <[email protected]> wrote:
> >
> > On Fri, Mar 18, 2022 at 11:01 AM Nick Desaulniers
> > <[email protected]> wrote:
> > >
> > > On Fri, Mar 18, 2022 at 7:04 AM Andy Shevchenko
> > > <[email protected]> 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
On Fri, Mar 18, 2022 at 11:01 AM Nick Desaulniers
<[email protected]> 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
On Fri, Mar 18, 2022 at 11:25 AM Bill Wendling <[email protected]> wrote:
>
> On Fri, Mar 18, 2022 at 11:01 AM Nick Desaulniers
> <[email protected]> wrote:
> >
> > On Fri, Mar 18, 2022 at 7:04 AM Andy Shevchenko
> > <[email protected]> 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
On Thu, Mar 17, 2022 at 11:11:21AM -0700, Nick Desaulniers wrote:
> On Thu, Mar 17, 2022 at 2:07 AM Andy Shevchenko
> <[email protected]> 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
On Fri, Mar 18, 2022 at 11:29 AM Nick Desaulniers
<[email protected]> 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
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 <[email protected]>
---
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
On Fri, Mar 18, 2022 at 7:04 AM Andy Shevchenko
<[email protected]> 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
On Fri, Mar 18, 2022 at 11:01 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Fri, Mar 18, 2022 at 7:04 AM Andy Shevchenko
> <[email protected]> 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
On Mon, Mar 21, 2022 at 3:09 AM Andy Shevchenko
<[email protected]> 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 <[email protected]>
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
On Fri, Mar 18, 2022 at 11:41 AM Linus Torvalds
<[email protected]> wrote:
>
> On Fri, Mar 18, 2022 at 11:29 AM Nick Desaulniers
> <[email protected]> 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