Linux: enable -Wformat

[nickdesaulniers]

A previous attempt at re-enabling -Wformat showed that there were LOTS of instances of this warning throughout the codebase. Simply re-applying that patch and doing an arm64 defconfig, I experienced 236 unique instances of this warning. Similar to @kees ' work enabling -Wvla, I'd like to find and fix all of these, then re-send the patch re-enabling the warning.

Assuming these aren't false positives, I think the only tricky cases we may encounter may come from config dependent types. ex.

#ifdef CONFIG_FOO
int bar = 0;
#else 
float bar = 0;
#endif 
printk("bar: %d\n"); // warning: format specifies type 'int' but the argument has type 'float' [-Wformat]

which can be fixed via:

+prink("bar: "
+#ifdef CONFIG_FOO
+    "%d"
+#else
+    "%f"
+#endif
+    "\n");
-printk("bar: %d\n");

(or whatever maintainers prefer). I don't expect many of those cases, but maybe I'm mistaken.

Also (meta), usually we file individual issues, but maybe it's better to use this one lone issue to track all the instances? (not sure; open to thoughts).

Also, let's find a way to split the work? And keep each other cc'ed when sending patches? (we should probably set up a mailing list to always cc for patches related to Clang Built Linux)

[nathanchance]

Also (meta), usually we file individual issues, but maybe it's better to use this one lone issue to track all the instances? (not sure; open to thoughts).

Given that some warnings might be easier fixed/received than others, it is still probably better to file individual issues and use the -Wformat label to link them all. We can either file issues as we deal with them or just take the list that I made above and file issues for all of them. I'd find the former easier to manage.

we should probably set up a mailing list to always cc for patches related to Clang Built Linux

I've been thinking this for a bit. I usually CC you but I am sure there are others who would like to be kept in the loop as well.

[nickdesaulniers]

https://groups.google.com/forum/#!forum/clang-built-linux

[nathanchance]

So the email we'd CC is [email protected]?

[nickdesaulniers]

it would be nice to get something on http://vger.kernel.org/vger-lists.html. I bet @arndb knows who to talk to about that.

[nathanchance]

Maybe emailing [email protected]?

[arndb]

On Mon, Feb 25, 2019 at 11:52 PM Nathan Chancellor [e-mail]
wrote:

Maybe emailing [email protected]?

Yes, that should be the right contact. Note that there is also a contact
for everything else on kernel.org,
but the mailing lists are handled by a separate team (David Miller is the
main person, not sure if
there are others).

   Arnd

[kragniz]

I submitted a patch, tell me if I did something wrong: https://patchwork.kernel.org/patch/10830941/

[jon-flatley]

It looks good to me. In the future could you CC Nick and I on the patches? That will help us stay organized and will let us add a quick Reviewed-by if a patch needs some traction.

[jon-flatley]

I reached out to the GCC community to try and better understand the difference for -Wformat in GCC vs Clang. It sounds like a big reason for the difference has to do with integer promotions. In GCC, any format specifier for a type that get promoted to an int checks the corresponding argument after it gets promoted, and just expects it to be an int after promotion. This means all types that get promoted to int, such as short int and char, are valid for any of each other's format specifiers in GCC. It appears that Clang checks these types pre-promotion, making Clang stricter.

So a side effect of fixing these format specifiers is that they will no longer be converted to the size of their original "incorrect" type specifiers when they're formatted. I'm not sure if that could be problematic in places, but just something to keep in mind.

[kragniz]

I submitted a patch, tell me if I did something wrong: https://patchwork.kernel.org/patch/10830941/

v2 of that patch: https://patchwork.kernel.org/patch/10831563/

[nickdesaulniers]

Some thoughts speaking more with @metti :

• Documentation/core-api/printk-formats.rst is useful to cite in commit messages.
• lib/vsprintf.c#format_decode() implements most of the format flag decoding.
• -Wformat-extra-args is another case of -Wformat that looks serious.

[kragniz]

Another patch: https://patchwork.kernel.org/patch/10832295/

[kragniz]

Some more patches:

https://patchwork.kernel.org/patch/10832955/
https://patchwork.kernel.org/patch/10833637/

[kragniz]

https://patchwork.ozlabs.org/patch/1050634/

[kragniz]

https://lore.kernel.org/patchwork/patch/1047450/

[andy-shev]

https://patchwork.ozlabs.org/patch/1050634/

This patch is wrong. We all know that IO port range is specified to be 0x0000-0xffff. You are trying to heal symptoms and not the issue.

[andy-shev]

Some more patches:

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

Not wrong, but better to use extended specifiers. I.e. here should be %16ph in use instead of all that old crap.

Joe did it in the right way.

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

Again, healing the symptoms. We all know from PCI specification that mention values are 16-bit wide.

[andy-shev]

I submitted a patch, tell me if I did something wrong: https://patchwork.kernel.org/patch/10830941/

v2 of that patch: https://patchwork.kernel.org/patch/10831563/

Healing the symptoms.

[nickdesaulniers]

It might be more useful to tackle non-integer-widening -Wformat errors first:
https://bugs.llvm.org/show_bug.cgi?id=41467
https://lkml.org/lkml/2019/4/11/639

[nickdesaulniers]

Looking into this more, I think GCC does not warn in one case, and it should, and the kernel should be fixed, too: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91206

But let's wait and see what GCC devs think.

https://bugs.llvm.org/show_bug.cgi?id=41467 is the LLVM bug report.

[jon-flatley]

It seems like this is mostly a difference in philosophy, and there is
no clear answer for if the compiler should warn when passing integer
types with dissimilar lengths to their respective format specifier.

I had this conversation with some members in the gcc community and the
argument was that warning on "incorrect" length specifiers would imply
that it was against the c spec, but this behavior is defined in
7.19.6.1:

h
Specifies that a following d, i, o, u, x, or X conversion specifier
applies to a short int or unsigned short int argument (the argument
will have been promoted according to the integer promotions, but
its value shall be converted to short int or unsigned short int
before printing); or that a following n conversion specifier applies
to a pointer to a short int argument.
...
If a conversion specification is invalid, the behavior is undefined.
If any argument is not the correct type for the corresponding
conversion specification, the behavior is undefined.

Now the answer to the question of whether or not the compiler should
warn in these situations lies in the interpretation of the language.
When asking if an argument "is not the correct type for the
corresponding conversion specification" is the spec referring to the
argument as it appears in the calling function, or as it is presented
to the called function? The language seems to indicate to me that the
correct type for the %hx format specifier for example would be
unsigned short int as it appears in the calling function, despite the
promotion to int within printf. This is up for debate, however.

On Thu, Jul 18, 2019 at 1:43 PM Nick Desaulniers
[e-mail] wrote:
>

Looking into this more, I think GCC does not warn in one case, and it should, and the kernel should be fixed, too: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91206

But let's wait and see what GCC devs think.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.

[nickdesaulniers]

@Nathan-Huckleberry has a patch to clang for the char -> short case: https://reviews.llvm.org/D66186

With that in, I think we can add a clang-version-specific check to scripts/Makefile.extrawarn that re-enables -Wformat for Clang if CONFIG_CLANG_VERSION >= 10.

[nickdesaulniers]

In https://reviews.llvm.org/rL369791, Nathan made [unsigned] char -> [unsigned]short warn only for -Wformat-pedantic, not -Wformat.

So I think the final thing we can likely do to close this out is a modification to scripts/Makefile.extrawarn that does the equivalent:

if clang:
  if clang_version < 10:
    KBUILD_CFLAGS += -Wno-format

or maybe:

if clang:
  if clang_version >= 10:
    KBUILD_CFLAGS += -Wformat -Wno-format-pedantic

[andy-shev]

if clang:
if clang_version >= 10:

Doesn't option check macros work on CLang? As far as I understand you may simple check for option independently on compiler (in case there is no collision between compilers for the same option name).

[nathanchance]

@andy-shev the issue is we don’t want to enable -Wformat for builds of clang before r369791 because it will be extremely noisy. -Wformat has been around for a while so just doing cc-option won’t work.

We could copy arch/arm64/kernel/vdso/Makefile:

ifeq ($(shell test $(CONFIG_CLANG_VERSION) -lt 100000; echo $$?),0)

[andy-shev]

@andy-shev the issue is we don’t want to enable -Wformat for builds of clang before r369791 because it will be extremely noisy. -Wformat has been around for a while so just doing cc-option won’t work.

I see.

We could copy arch/arm64/kernel/vdso/Makefile:

ifeq ($(shell test $(CONFIG_CLANG_VERSION) -lt 100000; echo $$?),0)

Perhaps making it global for all code makes sense.

[nickdesaulniers]

This is the patch I'd like to send for this:

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 4aea7cf71d11..4b0c0664b838 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -45,7 +45,11 @@ else

 ifdef CONFIG_CC_IS_CLANG
 KBUILD_CFLAGS += -Wno-initializer-overrides
+# Clang < 10 was too strict about -Wformat. -Wformat-security is enabled by
+# setting -Wformat, but we only want to enable it via W=1.
+ifeq ($(shell test $(CONFIG_CLANG_VERSION) -lt 100000; echo $$?),0)
 KBUILD_CFLAGS += -Wno-format
+endif
 KBUILD_CFLAGS += -Wno-sign-compare
 KBUILD_CFLAGS += -Wno-format-zero-length
 KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)

unfortunately, this seems to dig up some other instances that GCC does not, so even with
https://reviews.llvm.org/rL369791 we're still different. https://bugs.llvm.org/show_bug.cgi?id=41467

x86_64 defconfig on linux-next has 11 instances of warnings via -Wformat.

[arndb]

On Tue, May 5, 2020 at 2:11 AM Nick Desaulniers
[e-mail] wrote:
>

This is the patch I'd like to send for this:

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 4aea7cf71d11..4b0c0664b838 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -45,7 +45,11 @@ else

ifdef CONFIG_CC_IS_CLANG
KBUILD_CFLAGS += -Wno-initializer-overrides
+# Clang < 10 was too strict about -Wformat. -Wformat-security is enabled by
+# setting -Wformat, but we only want to enable it via W=1.
+ifeq ($(shell test $(CONFIG_CLANG_VERSION) -lt 100000; echo $$?),0)
KBUILD_CFLAGS += -Wno-format
+endif
KBUILD_CFLAGS += -Wno-sign-compare
KBUILD_CFLAGS += -Wno-format-zero-length
KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)

unfortunately, this seems to dig up some other instances that GCC does not, so even with
https://reviews.llvm.org/rL369791 we're still different. https://bugs.llvm.org/show_bug.cgi?id=41467

x86_64 defconfig on linux-next has 11 instances of warnings via -Wformat

I think number of files that show warnings in an allmodconfig build
would be more
important to list than the instances in defconfig.

I see 204 -Wformat warnings in 68 files on an x86 allmodconfig build with
clang-11, and they seem to all be the same kind of false positive, warning about
correct code:

  9  format specifies type 'char' but the argument has type 'int' [-Wformat]
  1  format specifies type 'char' but the argument has type 'u32'

(aka 'unsigned int') [-Wformat]
3 format specifies type 'char' but the argument has type
'unsigned int' [-Wformat]
2 format specifies type 'short' but the argument has type 'int'
[-Wformat]
1 format specifies type 'unsigned char' but the argument has
type '__u16' (aka 'unsigned short') [-Wformat]
20 format specifies type 'unsigned char' but the argument has
type 'int' [-Wformat]
1 format specifies type 'unsigned char' but the argument has
type 's16' (aka 'short') [-Wformat]
1 format specifies type 'unsigned char' but the argument has
type 's32' (aka 'int') [-Wformat]
12 format specifies type 'unsigned char' but the argument has
type 'u16' (aka 'unsigned short') [-Wformat]
4 format specifies type 'unsigned char' but the argument has
type 'u32' (aka 'unsigned int') [-Wformat]
5 format specifies type 'unsigned char' but the argument has
type 'unsigned int' [-Wformat]
1 format specifies type 'unsigned char' but the argument has
underlying type 'unsigned int' [-Wformat]
2 format specifies type 'unsigned short' but the argument has
type '__u32' (aka 'unsigned int') [-Wformat]
91 format specifies type 'unsigned short' but the argument has
type 'int' [-Wformat]
27 format specifies type 'unsigned short' but the argument has
type 'u32' (aka 'unsigned int') [-Wformat]
24 format specifies type 'unsigned short' but the argument has
type 'unsigned int' [-Wformat]

While it's possible to create a series of patches to fix them all up
(plus the ones that
are only seen in other configurations), I think a better workaround would be to
introduce a separate warning option in clang for the above that can be
turned off,
or just move them into -Wformat-pedantic like the subset from
https://bugs.llvm.org/show_bug.cgi?id=41467

  Arnd

[Nathan-Huckleberry]

The warnings leftover look valid to me. They all truncate before printing which seems like unintended behavior. From here https://lkml.org/lkml/2019/4/11/639 it looks like Linus thinks that using "hh" specifiers on an int is bad style.

I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95588

[nickdesaulniers]

Discussion on the list is that we should do a treewide removal of %h and %hh format modifiers for print like functions.
https://lore.kernel.org/lkml/[e-mail]/T/#m9795462050a04e8401a0ff4176760d189067316e

See also commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of unnecessary %h[xudi] and %hh[xudi]").

[nickdesaulniers]

Documentation in the kernel has been updated to reflect %h and %hh is frowned upon: https://lore.kernel.org/lkml/[email protected]/

Next up is separate efforts to add a warning to checkpatch.pl, and possibly leverage clang-tidy to help automate the removal of these format flags.