[PATCH] iwl4965: Enable checking of format strings

[PATCH] iwl4965: Enable checking of format strings

[Rasmus Villemoes]

Since these fmt_* variables are just const char*, and not const
char[], gcc (and smatch) doesn't to type checking of the arguments to
the printf functions. Since the linker knows perfectly well to merge
identical string constants, there's no point in having three static
pointers waste memory and give an extra level of indirection.

This removes over 100 "non-constant format argument" warnings from
smatch, accounting for about 20% of all such warnings in an
allmodconfig.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/net/wireless/iwlegacy/4965-debug.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/iwlegacy/4965-debug.c b/drivers/net/wireless/iwlegacy/4965-debug.c
index e0597bfdddb8..18855325cc1c 100644
--- a/drivers/net/wireless/iwlegacy/4965-debug.c
+++ b/drivers/net/wireless/iwlegacy/4965-debug.c
@@ -28,10 +28,9 @@
 #include "common.h"
 #include "4965.h"
 
-static const char *fmt_value = "  %-30s %10u\n";
-static const char *fmt_table = "  %-30s %10u  %10u  %10u  %10u\n";
-static const char *fmt_header =
-    "%-32s    current  cumulative       delta         max\n";
+#define fmt_value "  %-30s %10u\n"
+#define fmt_table "  %-30s %10u  %10u  %10u  %10u\n"
+#define fmt_header "%-32s    current  cumulative       delta         max\n"
 
 static int
 il4965_stats_flag(struct il_priv *il, char *buf, int bufsz)
-- 
2.1.3

Re: [PATCH] iwl4965: Enable checking of format strings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

On Feb 11, 2015, at 2:51 PM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Since these fmt_* variables are just const char*, and not const
> char[], gcc (and smatch) doesn't to type checking of the arguments to
> the printf functions. Since the linker knows perfectly well to merge
> identical string constants, there's no point in having three static
> pointers waste memory and give an extra level of indirection.
> 
> This removes over 100 "non-constant format argument" warnings from
> smatch, accounting for about 20% of all such warnings in an
> allmodconfig.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> drivers/net/wireless/iwlegacy/4965-debug.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/iwlegacy/4965-debug.c b/drivers/net/wireless/iwlegacy/4965-debug.c
> index e0597bfdddb8..18855325cc1c 100644
> --- a/drivers/net/wireless/iwlegacy/4965-debug.c
> +++ b/drivers/net/wireless/iwlegacy/4965-debug.c
> @@ -28,10 +28,9 @@
> #include "common.h"
> #include "4965.h"
> 
> -static const char *fmt_value = "  %-30s %10u\n";
> -static const char *fmt_table = "  %-30s %10u  %10u  %10u  %10u\n";
> -static const char *fmt_header =
> -    "%-32s    current  cumulative       delta         max\n";

Why not change these to:
static const char fmt_value[] = "  %-30s %10u\n";
static const char fmt_table[] = "  %-30s %10u  %10u  %10u  %10u\n";
static const char fmt_header[] =
    "%-32s    current  cumulative       delta         max\n";

I think that is better than the macros and avoids the extra pointers that I agree are useless.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] iwl4965: Enable checking of format strings

[Rasmus Villemoes]

On Thu, Feb 12 2015, "Rustad, Mark D" <mark.d.rustad@intel.com> wrote:

> On Feb 11, 2015, at 2:51 PM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
>> Since these fmt_* variables are just const char*, and not const
>> char[], gcc (and smatch) doesn't to type checking of the arguments to
>> the printf functions. Since the linker knows perfectly well to merge
>> identical string constants, there's no point in having three static
>> pointers waste memory and give an extra level of indirection.
>> 
>> This removes over 100 "non-constant format argument" warnings from
>> smatch, accounting for about 20% of all such warnings in an
>> allmodconfig.
>> 
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>> drivers/net/wireless/iwlegacy/4965-debug.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/iwlegacy/4965-debug.c b/drivers/net/wireless/iwlegacy/4965-debug.c
>> index e0597bfdddb8..18855325cc1c 100644
>> --- a/drivers/net/wireless/iwlegacy/4965-debug.c
>> +++ b/drivers/net/wireless/iwlegacy/4965-debug.c
>> @@ -28,10 +28,9 @@
>> #include "common.h"
>> #include "4965.h"
>> 
>> -static const char *fmt_value = "  %-30s %10u\n";
>> -static const char *fmt_table = "  %-30s %10u  %10u  %10u  %10u\n";
>> -static const char *fmt_header =
>> -    "%-32s    current  cumulative       delta         max\n";
>
> Why not change these to:
> static const char fmt_value[] = "  %-30s %10u\n";
> static const char fmt_table[] = "  %-30s %10u  %10u  %10u  %10u\n";
> static const char fmt_header[] =
>     "%-32s    current  cumulative       delta         max\n";
>
> I think that is better than the macros and avoids the extra pointers that I agree are useless.

Rather weak arguments, but I have three of them :-)

(1) If I'm reading some code and spot a non-constant format argument, I
sometimes track back to see how e.g. fmt_value is defined. If I then see
it's a macro, I immediately think "ok, the compiler is doing
type-checking". If it is a const char[], I have to remember that gcc
also does it in that case (as opposed to for example const char*const).

(2) The names of these variables themselves may end up wasting a few
bytes in the image.

(3) gcc/the linker doesn't merge identical const char[] arrays across
translation units. It also doesn't consider their tails for merging with
string literals. So although these specific strings are unlikely to
appear elsewhere, a string such as "%10u\n" or "max\n" couldn't be
merged with one of the above.

Rasmus

Re: [PATCH] iwl4965: Enable checking of format strings

[Mark Rustad]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/12/15 2:20 AM, Rasmus Villemoes wrote:
> Rather weak arguments, but I have three of them :-)

Yes, weak. All three.

> (1) If I'm reading some code and spot a non-constant format
> argument, I sometimes track back to see how e.g. fmt_value is
> defined. If I then see it's a macro, I immediately think "ok, the
> compiler is doing type-checking". If it is a const char[], I have
> to remember that gcc also does it in that case (as opposed to for
> example const char*const).

GCC should check in both cases. The case you are replacing was not
const char * const, but only const char *. Still, the compiler really
should check either form, even though theoretically the pointer in the
latter case could be changed, but the initial const value should be a
good indication of what the parameters are expected to be. No real
reason for the compiler not to check it.

> (2) The names of these variables themselves may end up wasting a
> few bytes in the image.

Maybe in a debug image, but they should be stripped from any normal
image. Really not a factor.

> (3) gcc/the linker doesn't merge identical const char[] arrays
> across translation units. It also doesn't consider their tails for
> merging with string literals. So although these specific strings
> are unlikely to appear elsewhere, a string such as "%10u\n" or
> "max\n" couldn't be merged with one of the above.

I haven't checked, but there is no theoretical reason that const char
[] items could not be merged exactly as the literals are. Considering
the boundaries the compiler guys push on optimization, doing such
merging would be tame by comparison (speculative stores make me crazy).
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.20 (Darwin)
Comment: GPGTools - http://gpgtools.org

iQIcBAEBAgAGBQJU3a3+AAoJEDwO/+eO4+5unvMP/jwxA4GmwC0d3VdGsVTJkMVd
zg+jwbkhnMiaEj6uPAwV5LXV/IGyuYFgNjoiNDg9RD3trV9/3YAxdAKw1ffO+PWe
lnmXSxaapLlCTapOsUdXPg88z9muQKrcfhnGyi+jt3BFeccXgtlHLsR0qVa4ddJw
KVHByPg+AlTSNzSnROxHH3UAbxEuZmDy+g+xfbEFLCKNCgtrSX5jGyG2vJIY3lhF
40VIdriUHz1QW4C1YYeJWMKwzml7Kln3u0T5MfDEtDfy6n7hiBhHczEgPjf7dnzd
aY4+VtKTyjPWLRhyDoJfR9maaV9TsYHpheSuUVzAGwvb85wH32ugdfmcW2RPnRfC
n9ThhtWd1WdJJpmq0xhLjc9bc3nrxJO8b2J/GMsT6SjGBhPGaaJSWY37UPhhOJOj
akKkA6QwD0u6Yoc3de7unGsiKWayD7e2k3w3bus+kCSspmyn/OnkzZRc0X3nXd20
suAWNZTalLWioqvI/hyvH3GMZxIuHTJoLRpTm+K7BQs7gBM9pD7OJOpn7XLtk2PM
zPfEj8fAUMV17lzFdBP+M+pGT3HzjWVwTIUgujdA4vL6eqB1W+3fR7kqjUuQYc69
aBaMde//i+HUPzTHZht2qXEb6K9EvSsz/XlhQrtAyu2gYY8PwchdZXH0NbAGqJ9C
4BEAO4HYJijd4vVSNBFO
=utge
-----END PGP SIGNATURE-----

Re: [PATCH] iwl4965: Enable checking of format strings

[Rasmus Villemoes]

On Fri, Feb 13 2015, Mark Rustad <mrustad@gmail.com> wrote:

> On 2/12/15 2:20 AM, Rasmus Villemoes wrote:
>> Rather weak arguments, but I have three of them :-)
>
> Yes, weak. All three.
>
>> (1) If I'm reading some code and spot a non-constant format
>> argument, I sometimes track back to see how e.g. fmt_value is
>> defined. If I then see it's a macro, I immediately think "ok, the
>> compiler is doing type-checking". If it is a const char[], I have
>> to remember that gcc also does it in that case (as opposed to for
>> example const char*const).
>
> GCC should check in both cases. The case you are replacing was not
> const char * const, but only const char *. Still, the compiler really
> should check either form, even though theoretically the pointer in the
> latter case could be changed, but the initial const value should be a
> good indication of what the parameters are expected to be. No real
> reason for the compiler not to check it.

I agree with all of that - just wanted to point out what gcc currently
does and doesn't, and changing to const char[] would indeed enable
checking.

>> (2) The names of these variables themselves may end up wasting a
>> few bytes in the image.
>
> Maybe in a debug image, but they should be stripped from any normal
> image. Really not a factor.

Sure, that was by far the weakest, and let's ignore that.

>> (3) gcc/the linker doesn't merge identical const char[] arrays
>> across translation units. It also doesn't consider their tails for
>> merging with string literals. So although these specific strings
>> are unlikely to appear elsewhere, a string such as "%10u\n" or
>> "max\n" couldn't be merged with one of the above.
>
> I haven't checked, but there is no theoretical reason that const char
> [] items could not be merged exactly as the literals are. Considering
> the boundaries the compiler guys push on optimization, doing such
> merging would be tame by comparison (speculative stores make me crazy).

Well, probably the linker is allowed to overlap "anonymous" objects
(string literals) with whatever const char[] (or indeed any const)
object it finds containing the appropriate byte sequence. But I think
language lawyers would insist that for

const char foo[] = "a string";
const char bar[] = "a string";

foo and bar have different addresses, whether they are defined in the
same or different TUs. One could then argue that if their address is
never taken explicitly it should be ok, but since passing foo to a printf
function effectively makes the address of foo escape the TU (even though
one is formally passing pointer to first element), I can certainly see
why compiler people would be reluctant to do merging of such objects.

Rasmus

RE: [PATCH] iwl4965: Enable checking of format strings

[David Laight]

From: Rasmus Villemoes
> Well, probably the linker is allowed to overlap "anonymous" objects
> (string literals) with whatever const char[] (or indeed any const)
> object it finds containing the appropriate byte sequence. But I think
> language lawyers would insist that for
> 
> const char foo[] = "a string";
> const char bar[] = "a string";

A quick test shows those are separate strings.
But 'const char *foo = "xxx";' will share.
You also need -O1 to get the strings into .rodata.str.n so that the linker
can merge them.

	David

Re: [PATCH] iwl4965: Enable checking of format strings

[Rasmus Villemoes]

On Fri, Feb 13 2015, David Laight <David.Laight@ACULAB.COM> wrote:

> From: Rasmus Villemoes
>> Well, probably the linker is allowed to overlap "anonymous" objects
>> (string literals) with whatever const char[] (or indeed any const)
>> object it finds containing the appropriate byte sequence. But I think
>> language lawyers would insist that for
>> 
>> const char foo[] = "a string";
>> const char bar[] = "a string";
>
> A quick test shows those are separate strings.
> But 'const char *foo = "xxx";' will share.

Yes, of course, because in that case you are actually creating two
objects, "xxx" which the linker will find some place to put, and foo
which is initialized to the address of whereever "xxx" was/will be
put. So one is wasting sizeof(const char*). Also, passing foo to a
function means the compiler has to load the value of foo and use that,
instead of simply passing the compile-time (well, link-time) constant
address of "xxx".

> You also need -O1 to get the strings into .rodata.str.n so that the linker
> can merge them.

Sure, optimization has to be turned on, but isn't the kernel always
compiled with -O2? ISTR that there are some things which won't even
work/compile with -O0.

Rasmus

Re: [PATCH] iwl4965: Enable checking of format strings

[Kalle Valo]

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:

> Since these fmt_* variables are just const char*, and not const
> char[], gcc (and smatch) doesn't to type checking of the arguments to
> the printf functions. Since the linker knows perfectly well to merge
> identical string constants, there's no point in having three static
> pointers waste memory and give an extra level of indirection.
>
> This removes over 100 "non-constant format argument" warnings from
> smatch, accounting for about 20% of all such warnings in an
> allmodconfig.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

So what's the conclusion, should I commit this patch or not?

Full discussion here:

https://patchwork.kernel.org/patch/5814811/

-- 
Kalle Valo

Re: [PATCH] iwl4965: Enable checking of format strings

[Stanislaw Gruszka]

On Tue, Apr 28, 2015 at 07:19:02PM +0300, Kalle Valo wrote:
> Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
> 
> > Since these fmt_* variables are just const char*, and not const
> > char[], gcc (and smatch) doesn't to type checking of the arguments to
> > the printf functions. Since the linker knows perfectly well to merge
> > identical string constants, there's no point in having three static
> > pointers waste memory and give an extra level of indirection.
> >
> > This removes over 100 "non-constant format argument" warnings from
> > smatch, accounting for about 20% of all such warnings in an
> > allmodconfig.
> >
> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> 
> So what's the conclusion, should I commit this patch or not?
> 
> Full discussion here:
> 
> https://patchwork.kernel.org/patch/5814811/

I do not see the point of the patch. If compiler behave not
optimally, fix the compiler. NACK.

Stanislaw