[cfe-dev] Relaxing format specifier checks

[Hans Wennborg via cfe-dev]

On Wed, May 16, 2018 at 11:52 PM, JF Bastien via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
>
> On May 16, 2018, at 10:41 AM, Shoaib Meenai via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
> The other thing to point out is, I'm dealing with codebases with *thousands*
> of such instances.
>
>
> This is why we care about this as well.

I haven't heard of any issues coming from this internally at Google or
in Chromium, so while I understand you have more Cocoa code, it can't
be that *everything* is broken by this.

> We've cleaned up a lot of them (and the clang fix-its do come in handy
> here), but at some point you just hit a wall and decide to go with
> -Wno-error=format to be able to move forward. I'm trying to avoid throwing
> the baby out with the bathwater here by adding an option to still retain
> some of the format errors, at least. My plan is to actually go back and
> clean up the rest of the issues as well, but this would allow us to do it in
> a staged fashion instead of an all-or-nothing.

It sounds like a -Wformat-relaxed variant would serve this approach.

>> It sounds like the problem is really that NSInteger and size_t are
>> typedeffed to different types on 32-bit. Would it be possible to get
>> that fixed instead?
>
>
> NSInteger is, on purpose, the same size as a size_t. The types can’t change.

(It's a shame they weren't typedefed to the same type then; but I
suppose I should let that go.)

Is this documented somewhere? As pointed out on the review, Apple
recommends casting and printing NSInteger with %ld. Where does the
practice of using %zd come from?

Anyway, this proposal is much broader than handling the NSInteger vs
%zd issue, as it would suppress all kinds of argument mismatches as
long as the size and alignment matches on the target. I think that
would be very unfortunate, but I can see a place for an off-by-default
relaxed variant of the warning for code that doesn't care about it.

>> It's (mostly) not trying to be clever; it's just checking what the
>> standard requires. Printing an int with %ld is wrong according to the
>> standard even if int and long are the same size on the target.
>
>
> Many things are wrong according to the standard and we don’t warn about it
> because the standard is being silly. In this case I don’t see a technical
> reason why integral types with matching sizeof and signed-ness should be
> incompatible. There is a portability problem if you recompile and the
> platform doesn’t make sure the format specifier and integral types aren’t in
> sync, but as explained in the patch that’s guaranteed to work on Darwin
> platforms.

So the argument is that code compiled on Darwin doesn't care about
portability? I don't think the standard is being particularly silly
here and I don't expect it to change.

>> I prefer to think of them as correctness warnings rather than
>> portability warnings. I'm not buying the argument that because it
>> doesn't catch all portability issues, it shouldn't warn about
>> incorrect code.
>
>
> In the very specific case of NSInteger and %z on Darwin platforms, do we
> agree there’s no correctness issue? I’m not talking standards correctness,
> I’m talking “the code does what’s expected”.

Sure.

But the current proposal seems much broader than that. And it seems
like it would be a shame to hack around just this specific issue in
the compiler.

>> I would prefer not to relax -Wformat by default. I think what it's
>> doing is simple and correct, and a large amount of code compiles with
>> it.
>
>
> We’d be relaxing something that only just got added to clang, and which
> tries to catch portability issues.

-Wformat has been in a long time. From what I understand it's just
that it recently started caring about signed %z variants.

> It’s evidently not doing this very well
> because it doesn’t know about platform guarantees. It’s a useful warning in
> general, but the false positive rate is high, at least on Darwin platforms.
> That’s why I want to “relax” -Wformat: because it’s noisy and people will
> ignore it.

It's been doing well so far. It sounds like the false positive rate is
due to a specific problematic problem on a specific platform, so it
seems a shame to downgrade the warning overall just because of that.

>> I'm not opposed to adding a -Wformat-relaxed option if there's a lot
>> of demand for it.
>
>
> I’m opposed to having these format portability warnings on with -Wall. -Wall
> shouldn’t be erroneously noisy.

As above, it seems the noise is contained to a single problem on a
single platform. I think the warning overall is high quality.

Cheers,
Hans