LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] afs: use correct format characters
@ 2019-04-10 22:03 Louis Taylor
  2019-04-10 22:27 ` Nick Desaulniers
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Louis Taylor @ 2019-04-10 22:03 UTC (permalink / raw)
  To: dhowells; +Cc: linux-afs, linux-kernel, clang-built-linux, Louis Taylor

When compiling with -Wformat, clang warns:

fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
      'unsigned char' [-Wformat]
        _leave(" = %d [%hd]", ret, fl->fl_type);
                       ~~~         ^~~~~~~~~~~

fl_type is declared as an unsigned char unconditionally in
include/linux/fs.h, so use the correct format characters.

Link: https://github.com/ClangBuiltLinux/linux/issues/378
Signed-off-by: Louis Taylor <louis@kragniz.eu>
---
 fs/afs/flock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/afs/flock.c b/fs/afs/flock.c
index 6a0174258382..be4c3f6a3178 100644
--- a/fs/afs/flock.c
+++ b/fs/afs/flock.c
@@ -629,7 +629,7 @@ static int afs_do_getlk(struct file *file, struct file_lock *fl)
 
 	ret = 0;
 error:
-	_leave(" = %d [%hd]", ret, fl->fl_type);
+	_leave(" = %d [%hhu]", ret, fl->fl_type);
 	return ret;
 }
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] afs: use correct format characters
  2019-04-10 22:03 [PATCH] afs: use correct format characters Louis Taylor
@ 2019-04-10 22:27 ` Nick Desaulniers
  2019-04-10 22:41 ` [PATCH v2] " Louis Taylor
  2019-04-10 23:00 ` [PATCH] " Joe Perches
  2 siblings, 0 replies; 8+ messages in thread
From: Nick Desaulniers @ 2019-04-10 22:27 UTC (permalink / raw)
  To: Louis Taylor; +Cc: David Howells, linux-afs, LKML, clang-built-linux

On Wed, Apr 10, 2019 at 3:03 PM Louis Taylor <louis@kragniz.eu> wrote:
>
> When compiling with -Wformat, clang warns:
>
> fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
>       'unsigned char' [-Wformat]
>         _leave(" = %d [%hd]", ret, fl->fl_type);
>                        ~~~         ^~~~~~~~~~~
>
> fl_type is declared as an unsigned char unconditionally in
> include/linux/fs.h, so use the correct format characters.

Thanks for the patch, LGTM.  I had in my notes that there's a similar warning in
fs/afs/dir.c
line 138
format specifies type 'unsigned short' but the argument has type 'int'

can you verify if that's still the case, and if so, roll that change
into this one in a v2?

Otherwise, I'll post my reviewed by tag, and we can take just this one.

>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <louis@kragniz.eu>
> ---
>  fs/afs/flock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/afs/flock.c b/fs/afs/flock.c
> index 6a0174258382..be4c3f6a3178 100644
> --- a/fs/afs/flock.c
> +++ b/fs/afs/flock.c
> @@ -629,7 +629,7 @@ static int afs_do_getlk(struct file *file, struct file_lock *fl)
>
>         ret = 0;
>  error:
> -       _leave(" = %d [%hd]", ret, fl->fl_type);
> +       _leave(" = %d [%hhu]", ret, fl->fl_type);
>         return ret;
>  }
>
> --
> 2.21.0
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To post to this group, send email to clang-built-linux@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20190410220301.2332-1-louis%40kragniz.eu.
> For more options, visit https://groups.google.com/d/optout.



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] afs: use correct format characters
  2019-04-10 22:03 [PATCH] afs: use correct format characters Louis Taylor
  2019-04-10 22:27 ` Nick Desaulniers
@ 2019-04-10 22:41 ` Louis Taylor
  2019-04-10 22:52   ` Nick Desaulniers
  2019-04-10 23:00 ` [PATCH] " Joe Perches
  2 siblings, 1 reply; 8+ messages in thread
From: Louis Taylor @ 2019-04-10 22:41 UTC (permalink / raw)
  To: dhowells; +Cc: linux-afs, linux-kernel, clang-built-linux, Louis Taylor

When compiling with -Wformat, clang warns:

fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
      'unsigned char' [-Wformat]
        _leave(" = %d [%hd]", ret, fl->fl_type);
                       ~~~         ^~~~~~~~~~~

fs/afs/dir.c:138:11: warning: format specifies type 'unsigned short' but
the argument has type 'int' [-Wformat]
       ntohs(dbuf->blocks[tmp].hdr.magic));
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

fl_type is declared as an unsigned char unconditionally in
include/linux/fs.h, so use the correct format characters.

Link: https://github.com/ClangBuiltLinux/linux/issues/378
Signed-off-by: Louis Taylor <louis@kragniz.eu>
---
 fs/afs/dir.c   | 2 +-
 fs/afs/flock.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 8a2562e3a316..4ceaec94e9c5 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -133,7 +133,7 @@ static bool afs_dir_check_page(struct afs_vnode *dvnode, struct page *page,
 	dbuf = kmap(page);
 	for (tmp = 0; tmp < qty; tmp++) {
 		if (dbuf->blocks[tmp].hdr.magic != AFS_DIR_MAGIC) {
-			printk("kAFS: %s(%lx): bad magic %d/%d is %04hx\n",
+			printk("kAFS: %s(%lx): bad magic %d/%d is %04x\n",
 			       __func__, dvnode->vfs_inode.i_ino, tmp, qty,
 			       ntohs(dbuf->blocks[tmp].hdr.magic));
 			trace_afs_dir_check_failed(dvnode, off, i_size);
diff --git a/fs/afs/flock.c b/fs/afs/flock.c
index 6a0174258382..be4c3f6a3178 100644
--- a/fs/afs/flock.c
+++ b/fs/afs/flock.c
@@ -629,7 +629,7 @@ static int afs_do_getlk(struct file *file, struct file_lock *fl)
 
 	ret = 0;
 error:
-	_leave(" = %d [%hd]", ret, fl->fl_type);
+	_leave(" = %d [%hhu]", ret, fl->fl_type);
 	return ret;
 }
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] afs: use correct format characters
  2019-04-10 22:41 ` [PATCH v2] " Louis Taylor
@ 2019-04-10 22:52   ` Nick Desaulniers
  2019-04-12 15:48     ` David Laight
  0 siblings, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2019-04-10 22:52 UTC (permalink / raw)
  To: Louis Taylor; +Cc: David Howells, linux-afs, LKML, clang-built-linux

On Wed, Apr 10, 2019 at 3:41 PM Louis Taylor <louis@kragniz.eu> wrote:
>
> When compiling with -Wformat, clang warns:
>
> fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
>       'unsigned char' [-Wformat]
>         _leave(" = %d [%hd]", ret, fl->fl_type);
>                        ~~~         ^~~~~~~~~~~
>
> fs/afs/dir.c:138:11: warning: format specifies type 'unsigned short' but
> the argument has type 'int' [-Wformat]
>        ntohs(dbuf->blocks[tmp].hdr.magic));
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> fl_type is declared as an unsigned char unconditionally in
> include/linux/fs.h, so use the correct format characters.

Thanks for the v2, probably should include a note about ntohs.  That
case in particular looks more complicated, due to the definition of
ntohs (which uses __swab16).

If you keep the previous flag of %04hx, but add an explicit cast to
u16, does the warning go away? If so, that might be a better fix.

- ntohs(dbuf->blocks[tmp].hdr.magic));
+ (u16)ntohs(dbuf->blocks[tmp].hdr.magic));

?

Particularly, I'm curious about the return type of GNU C statement
expressions, in the definition of __swab16 if __HAVE_BUILTIN_BSWAP16__
is not defined.

>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <louis@kragniz.eu>
> ---
>  fs/afs/dir.c   | 2 +-
>  fs/afs/flock.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index 8a2562e3a316..4ceaec94e9c5 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -133,7 +133,7 @@ static bool afs_dir_check_page(struct afs_vnode *dvnode, struct page *page,
>         dbuf = kmap(page);
>         for (tmp = 0; tmp < qty; tmp++) {
>                 if (dbuf->blocks[tmp].hdr.magic != AFS_DIR_MAGIC) {
> -                       printk("kAFS: %s(%lx): bad magic %d/%d is %04hx\n",
> +                       printk("kAFS: %s(%lx): bad magic %d/%d is %04x\n",
>                                __func__, dvnode->vfs_inode.i_ino, tmp, qty,
>                                ntohs(dbuf->blocks[tmp].hdr.magic));
>                         trace_afs_dir_check_failed(dvnode, off, i_size);
> diff --git a/fs/afs/flock.c b/fs/afs/flock.c
> index 6a0174258382..be4c3f6a3178 100644
> --- a/fs/afs/flock.c
> +++ b/fs/afs/flock.c
> @@ -629,7 +629,7 @@ static int afs_do_getlk(struct file *file, struct file_lock *fl)
>
>         ret = 0;
>  error:
> -       _leave(" = %d [%hd]", ret, fl->fl_type);
> +       _leave(" = %d [%hhu]", ret, fl->fl_type);
>         return ret;
>  }

-
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] afs: use correct format characters
  2019-04-10 22:03 [PATCH] afs: use correct format characters Louis Taylor
  2019-04-10 22:27 ` Nick Desaulniers
  2019-04-10 22:41 ` [PATCH v2] " Louis Taylor
@ 2019-04-10 23:00 ` Joe Perches
  2019-04-11 16:31   ` Linus Torvalds
  2 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2019-04-10 23:00 UTC (permalink / raw)
  To: Louis Taylor, dhowells, Linus Torvalds
  Cc: linux-afs, linux-kernel, clang-built-linux

On Wed, 2019-04-10 at 23:03 +0100, Louis Taylor wrote:
> When compiling with -Wformat, clang warns:
> 
> fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
>       'unsigned char' [-Wformat]
>         _leave(" = %d [%hd]", ret, fl->fl_type);

I really think this clang message should be ignored.

It's really unnecessary as every vararg argument smaller
than int size is already promoted to int.

This particular error is pedantic and has no effect
_at all_ on output or runtime.

If there was some actual mismatch between the signedness
of the argument and the format type, it could make sense.

ie:

	signed char foo = (signed char)-1;
	printk("mismatched %%d emitted as %%u: %u\n", foo);

where the output is a somewhat unexpected 4294967295

> fl_type is declared as an unsigned char unconditionally in
> include/linux/fs.h, so use the correct format characters.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <louis@kragniz.eu>
> ---
>  fs/afs/flock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/afs/flock.c b/fs/afs/flock.c
> index 6a0174258382..be4c3f6a3178 100644
> --- a/fs/afs/flock.c
> +++ b/fs/afs/flock.c
> @@ -629,7 +629,7 @@ static int afs_do_getlk(struct file *file, struct file_lock *fl)
>  
>  	ret = 0;
>  error:
> -	_leave(" = %d [%hd]", ret, fl->fl_type);
> +	_leave(" = %d [%hhu]", ret, fl->fl_type);
>  	return ret;
>  }
>  


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] afs: use correct format characters
  2019-04-10 23:00 ` [PATCH] " Joe Perches
@ 2019-04-11 16:31   ` Linus Torvalds
  2019-04-11 17:41     ` Nick Desaulniers
  0 siblings, 1 reply; 8+ messages in thread
From: Linus Torvalds @ 2019-04-11 16:31 UTC (permalink / raw)
  To: Joe Perches
  Cc: Louis Taylor, David Howells, linux-afs,
	Linux List Kernel Mailing, clang-built-linux

On Wed, Apr 10, 2019 at 4:01 PM Joe Perches <joe@perches.com> wrote:
>
> I really think this clang message should be ignored.

Agreed.

> It's really unnecessary as every vararg argument smaller
> than int size is already promoted to int.

Exactly. It's a pointless warning, making for more complex code, and
making people remember esoteric printf format details that have no
reason for existing.

The "h" and "hh" things should never be used. The only reason for them
being used if if you have an "int", but you want to print it out as a
"char" (and honestly, that is a really bad reason, you'd be better off
just using a proper cast to make the code more obvious).

So if what you have a "char" (or unsigned char) you should always just
print it out as an "int", knowing that the compiler already did the
proper type conversion.

                             Linus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] afs: use correct format characters
  2019-04-11 16:31   ` Linus Torvalds
@ 2019-04-11 17:41     ` Nick Desaulniers
  0 siblings, 0 replies; 8+ messages in thread
From: Nick Desaulniers @ 2019-04-11 17:41 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Joe Perches, Louis Taylor, David Howells, linux-afs,
	Linux List Kernel Mailing, clang-built-linux

On Thu, Apr 11, 2019 at 9:31 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Apr 10, 2019 at 4:01 PM Joe Perches <joe@perches.com> wrote:
> >
> > I really think this clang message should be ignored.
>
> Agreed.
>
> > It's really unnecessary as every vararg argument smaller
> > than int size is already promoted to int.
>
> Exactly. It's a pointless warning, making for more complex code, and
> making people remember esoteric printf format details that have no
> reason for existing.
>
> The "h" and "hh" things should never be used. The only reason for them
> being used if if you have an "int", but you want to print it out as a
> "char" (and honestly, that is a really bad reason, you'd be better off
> just using a proper cast to make the code more obvious).
>
> So if what you have a "char" (or unsigned char) you should always just
> print it out as an "int", knowing that the compiler already did the
> proper type conversion.
>
>                              Linus

https://bugs.llvm.org/show_bug.cgi?id=41467

I still think -Wformat helpful for catching completely nonsensical
format strings like printing a floating point type as an integral
type, or not having the correct number of arguments for the number of
format strings.  We'll take a look to see if we can differentiate
between those and these "integer widening" ones better.
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v2] afs: use correct format characters
  2019-04-10 22:52   ` Nick Desaulniers
@ 2019-04-12 15:48     ` David Laight
  0 siblings, 0 replies; 8+ messages in thread
From: David Laight @ 2019-04-12 15:48 UTC (permalink / raw)
  To: 'Nick Desaulniers', Louis Taylor
  Cc: David Howells, linux-afs, LKML, clang-built-linux

From: Nick Desaulniers
> Sent: 10 April 2019 23:52
> On Wed, Apr 10, 2019 at 3:41 PM Louis Taylor <louis@kragniz.eu> wrote:
> >
> > When compiling with -Wformat, clang warns:
> >
> > fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
> >       'unsigned char' [-Wformat]
> >         _leave(" = %d [%hd]", ret, fl->fl_type);
> >                        ~~~         ^~~~~~~~~~~
> >
> > fs/afs/dir.c:138:11: warning: format specifies type 'unsigned short' but
> > the argument has type 'int' [-Wformat]
> >        ntohs(dbuf->blocks[tmp].hdr.magic));
> >        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > fl_type is declared as an unsigned char unconditionally in
> > include/linux/fs.h, so use the correct format characters.
> 
> Thanks for the v2, probably should include a note about ntohs.  That
> case in particular looks more complicated, due to the definition of
> ntohs (which uses __swab16).
> 
> If you keep the previous flag of %04hx, but add an explicit cast to
> u16, does the warning go away? If so, that might be a better fix.
> 
> - ntohs(dbuf->blocks[tmp].hdr.magic));
> + (u16)ntohs(dbuf->blocks[tmp].hdr.magic));

Or just append '+ 0u' forcing the type to 'unsigned int'.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-10 22:03 [PATCH] afs: use correct format characters Louis Taylor
2019-04-10 22:27 ` Nick Desaulniers
2019-04-10 22:41 ` [PATCH v2] " Louis Taylor
2019-04-10 22:52   ` Nick Desaulniers
2019-04-12 15:48     ` David Laight
2019-04-10 23:00 ` [PATCH] " Joe Perches
2019-04-11 16:31   ` Linus Torvalds
2019-04-11 17:41     ` Nick Desaulniers

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git