[clang][-Wformat] consider moving truncating format flags to -Wformat-pedantic

[nickdesaulniers]

Vaguely reminiscent of #40812.

We've been having a hard time enabling -Wformat for clang within the Linux kernel.
ClangBuiltLinux/linux#378
https://lore.kernel.org/llvm/CAHk-=wivP4zipYnwNWCLF5cd24GLs3m8=Sp7M-CmmPva_UC+3Q@mail.gmail.com/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=21f9c8a13bb2a0c24d9c6b86bc0896542a28c197https://lore.kernel.org/llvm/CAHk-=wgJA=e-CLcvU5LRKu0bMLeAewXtOM6as1hFVeQAVkMPbg@mail.gmail.com/

Consider the following hypothetical test case:

#include <stdio.h>

void bar (int unused, int x) {
     // -Wformat: char format, arg is int
    printf("bottom word is %hhd\n", x);
}

When compiled with -Wformat -O2 -mskip-rax-setup (-mskip-rax-setup just to make the output marginally clearer, -mskip-rax-setup is used by the linux kernel.) we get the following warning:

warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
    printf("bottom word is %hhd\n", x);
                           ~~~~     ^
                           %d

And produces the following disassembly:

bar:                                    # @bar
        movl    $.L.str, %edi
        xorl    %eax, %eax
        jmp     printf                          # TAILCALL
.L.str:
        .asciz  "bottom word is %hhd\n"

Let's consider informing the compiler that this was intentioned by adding an explicit cast to show we meant to truncate in the caller.

 #include <stdio.h>
 
 void bar (int unused, int x) {
     // -Wformat: char format, arg is int
-    printf("bottom word is %hhd\n", x);
+    printf("bottom word is %hhd\n", (signed char)x);
 }

This silences the warning, and shows intent. Maybe we add a comment for reviewers + maintainers to understand we intend to truncate the value here.

Oh, but look at the disassembly:

 bar:
+        movzbl  %sil, %esi
         movl    $.L.str, %edi
         xorl    %eax, %eax
         jmp     printf  

This is adding overhead to the caller, when the callee (printf) is still going to just look at the first byte due to the continued use of %hhd format flag. That's kind of a waste.

Here's a diff of the llvm ir as well:

define dso_local void @foo(i32 noundef %0, i32 noundef %1) local_unnamed_addr #0 {
-  %2 = tail call i32 (i8*, ...) @printf(i8* noundef nonnull dereferenceable(1) getelementptr inbounds ([21 x i8], [21 x i8]* @.str, i64 0, i64 0), i32 noundef %1)
+ %3 = and i32 %1, 255
+ %4 = tail call i32 (i8*, ...) @printf(i8* noundef nonnull dereferenceable(1) getelementptr inbounds ([21 x i8], [21 x i8]* @.str, i64 0, i64 0), i32 noundef %3)
  ret void
}

The and is optimized, without optimizations we have:

  %6 = trunc i32 %5 to i8
  %7 = zext i8 %6 to i32

(ie. truncate then default argument promote)

One might argue "just declare x as signed char then." But I think we have quite a few use cases in the kernel (primarily debugging) where we might want to retain a full words (or more) worth of storage but then print individual bytes.

Can we consider moving truncating format instances of -Wformat to either -Wformat-pedantic or perhaps a new sub-flag in -Wformat group?

cc @zygoloid @AaronBallman

CheckPrintfHandler::checkFormatExpr is the method in clang that does the format flag warning logic. We might be able to consider moving some more cases to -Wformat-pedantic.

[AaronBallman]

Can we consider moving truncating format instances of -Wformat to either -Wformat-pedantic or perhaps a new sub-flag in -Wformat group?

WG14 adopted N2562 for C2x to clarify exactly these situations and we should update -Wformat accordingly. While these are normative changes to C2x, they're basically clarifications of intent and so I think we could consider applying the changes in all language modes.

[llvmbot]

@llvm/issue-subscribers-c2x

[torvalds]

Note that this issue is much more important than some "extra code generation".

The warning is actively completely wrong, and bogus enough that this has caused the Linux kernel to turn off -Wformat for clang, because clang developers are clearly not understanding the meaning of the format.

Imagine that you have a character value, and you want to print out its integer value. What would you use? (Note: I say "character value", not "variable of type 'char'". See later).

Normally, you'd just use the regular '%d , %u or %x variations. It's simple, it's the "print integer" format, it's the one people know. That takes care of all the usual case.

So why would you ever use '%hhu or %hhd?

You'd use them because you very explicitly want to only print the unsigned or signed 'char' value. IOW, the main reason to use a format like '%hhd' is that you are very aware that you are doing something that is explicitly about the low bits of the value, and you want to avoid having to cast the argument.

For example, let's say that you actually know what you are doing, and you want to print out the hex value of a byte in a 'char' string. You are also very aware that 'char' has an implementation-defined sign.

You have basically two valid ways to do this:

printf("%02x\n", (unsigned char) c);
printf("%02hhx\n", c);

where the first one just says "print it as a an integer value that I've explicitly converted into the form I want", while the other one says "print it as an unsigned char".

IOW, the whole - and really ONLY - point of having that 'hh' modifier is literally to avoid the cast.

This is from the C standard (ok, my google search found some 2007 pdf, maybe it's changed since):

    The length modifiers and their meanings are:

    hh      Specifies that a following d, i, o, u, x, or X conversion specifier
            applies to a signed char or unsigned char argument (the argument will
            have been promoted according to the integer promotions, but its value
            shall be converted to signed char or unsigned char before printing);
            or that a following n conversion specifier applies to a pointer to a
            signed char argument.

IOW, the standard makes it very clear that the argument will have gone through all the usual integer promotions, but it then for printing will be converted to signed or unsigned char.

Read that again: the whole documented point of the 'hh' specifier is basically to avoid the manual cast..

So clang basically warning and saying that

    format specifies type 'char' but the argument has type 'int'

is basically negating the documented raison d'être of the 'hh' modifier. The standard literally talks about how the argument will be promoted by the usual integer promotions (ie it will never actually be passed as 'char'), but that printf will then convert it to the wanted type.

A compiler writer that thinks the user should do the cast to 'char' before using '%hhd' is a compiler writer that fundamentally does not understand what %hhd is all about!

Note that there are a few different classes of 'char' use: one is obviously as the building block of 'string' values, but the printf format for that is '%s' and '%c'. When you use '%hhd' or '%hhu' it's typically because you use 'char' for the other reason it's often used: an integer value that packs well in memory.

And in both cases, you often end up using 'int' for the calculation parts. You may want the in-memory representation to be 'char' - whether as part of a string or whether you want a compact memory representation - but then when you use the value you will quite typically use an 'int' variable for it.

In many cases, that's actually what the C standard requires.

For example, very basic and traditional C functions like 'getch()' and 'isalpha()' explicitly do not work with a 'char' value. The standard makes it very very clear that those functions have 'int' return values and arguments respectively. So expressing character values as 'int' is traditional and normal and expected. It's just how C works.

Even a 'character constant' is of type 'int'.

So then having a compiler that complains when you want to print out said 'int' value using '%hhc' and says "I wanted a char type", that compiler is simply hot garbage and completely wrong.

Please fix this completely bogus warning in clang, so that we can enable -Wformat for the kernel again, instead of having to go "the clang developers don't understand format strings, and warn about valid things, so we'll disable their incompetent warnings"

[AaronBallman]

Note that this issue is much more important than some "extra code generation".

Agreed; overly chatty diagnostics don't do anyone any good.

The warning is actively completely wrong, and bogus enough that this has caused the Linux kernel to turn off -Wformat for clang,

That's exactly the situation we want to avoid.

However, as pointed out in a previous comment, WG14 needed to clarify this situation because other parts of the standard than the ones you've cited claimed undefined behavior in these cases. The confusion largely came from C17 7.21.6.1p9, which said "If any argument is not the correct type for the corresponding conversion specification, the behavior is undefined." Note that %hhd isn't a conversion specification, it's a length modifier followed by a conversion specification, and the conversion specification for d says "the int argument", so there's already some room for interpretation in the standard. Then consider a type like _Bool which has no conversion specification at all. With that in mind, you can see why the warning was not completely wrong (users like knowing about UB in their code before the optimizer hurts them with it), but it was certainly quite pedantic given the common C standard library implementation practices.

WG14 didn't intend for these kinds of situations to be undefined behavior; that's not helpful nor is it what users expect. So we
clarified the standard in N2562 for C2x, and as I mentioned above, the plan in Clang is to apply that correction to all C language modes as the paper was standardizing existing practice. Those clarifications are in line with the behavior you'd like to see implemented in Clang, so the end result when we fix this should significantly reduce the chattiness you're seeing.

...because clang developers are clearly not understanding the meaning of the format.

...that compiler is simply hot garbage and completely wrong.

... instead of having to go "the clang developers don't understand format strings, and warn about valid things, so we'll disable their incompetent warnings"

Please familiarize yourself with our community Code of Conduct and refrain from such language in the future.

[torvalds]

WG14 didn't intend for these kinds of situations to be undefined behavior; that's not helpful nor is it what users expect. So we clarified the standard in N2562 for C2x, and as I mentioned above, the plan in Clang is to apply that correction to all C language modes as the paper was standardizing existing practice. Those clarifications are in line with the behavior you'd like to see implemented in Clang, so the end result when we fix this should significantly reduce the chattiness you're seeing.

Thanks.

The actual warnings in the kernel are easy to avoid - using 'hh' as a modifier is relatively rare (they tend to cluster in a few drivers and subsystems - I think about half of them are in the mediatek wireless driver). And the most common case by far is just "print a 'char' without worries about sign", so the ones that warn tend to be odd outliers where we can just remove 'hh'.

But they keep happening, and I can't blame developers for them when it's clearly just clang being odd (and people tested their code with a compiler that wasn't odd).

And while they are not hard to fix as they pop up, it's a whack-a-mole game that I'm not willing to play if there is no sign of clang being fixed.

Now, with a compiler fix, it then typically takes years for that fix to actually percolate out (we used to support compiler releases over a decade old, we've tried to reign that in a bit and it's hopefully less than that now, but many distributions are very slow to upgrade compilers because it can be so painful).

So even with a fix in the pipeline, we'd have to live with the old warning for a while, but if there's a fix in the pipeline it's at least not nearly as frustrating to have to deal with.

I'm perfectly willing to deal with compiler bugs and treat them as workarounds - it can be painful, but it happens. But if it's an intentional compiler "feature" that isn't expected to be fixed, I just disable it (classic example: I absolutely despise the type-based C alias rules, and refuse to have anything to do with that disgusting mistake by the C language committee).

Please familiarize yourself with our community Code of Conduct and refrain from such language in the future.

Apologies.

This has been very frustrating. We've literally disabled -Wformat on clang since 2012 because it's been too noisy. People have worked on trying to make clang builds be widely usable in general for the kernel, and be warning-free for a long time, and we're this close ("holds up two fingers almost touching").

The -Wformat thing actually got re-enabled finally for clang (people had worked to remove all warning causes), and then the very same merge window that the warning got re-enabled, I think we had two or three new cases of it pop up in two different drivers.

And they are all "fixable" (and they may already be fixed in my tree), but this one in particular is so frustrating because -Wformat is very useful, but it's simply not worth fighting a compiler that most developers don't use and that gives false positives. The fixes are also often quite silly, with things like "Oh, we mix %hhu and %d randomly in the same format string depending on just how we generated the value, and whether it was just a single variable of type 'unsigned char', or whether it was an expression that had the implicit widening to 'int'".