[cfe-dev] Relaxing format specifier checks

Wed May 16 10:41:15 PDT 2018

I don't think making NSInteger and size_t consistent at this point is an option, because of the ABI implications, but I'll leave that for the Apple folks to answer.

The other thing to point out is, I'm dealing with codebases with *thousands* of such instances. 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.

From: cfe-dev <cfe-dev-bounces at lists.llvm.org> on behalf of cfe-dev <cfe-dev at lists.llvm.org>
Reply-To: Nico Weber <thakis at chromium.org>
Date: Wednesday, May 16, 2018 at 5:33 AM
To: Aaron Ballman <aaron at aaronballman.com>
Cc: cfe-dev <cfe-dev at lists.llvm.org>
Subject: Re: [cfe-dev] Relaxing format specifier checks

Another +1 to that.

For NSInteger, one thing that projects can do until the SDK either makes NSInteger and ssize_t consistent or offers format strings for NSInteger instead of recommending casting is to add something like this and use it:

// The size of NSInteger and NSUInteger varies between 32-bit and 64-bit
// architectures and Apple does not provides standard format macros and
// recommends casting. This has many drawbacks, so instead define macros
// for formatting those types.
#if defined(OS_MACOSX)
#if defined(ARCH_CPU_64_BITS)
#if !defined(PRIdNS)
#define PRIdNS "ld"
#endif
#if !defined(PRIuNS)
#define PRIuNS "lu"
#endif
#if !defined(PRIxNS)
#define PRIxNS "lx"
#endif
#else  // defined(ARCH_CPU_64_BITS)
#if !defined(PRIdNS)
#define PRIdNS "d"
#endif
#if !defined(PRIuNS)
#define PRIuNS "u"
#endif
#if !defined(PRIxNS)
#define PRIxNS "x"
#endif
#endif
#endif  // defined(OS_MACOSX)

On Wed, May 16, 2018 at 7:49 AM, Aaron Ballman via cfe-dev <cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>> wrote:
I'm in the same camp as Hans and Joerg -- I would prefer to keep the
existing check functionality and provide a more relaxed mode as an
alternative if one is necessary (for the same reasons they bring up).

~Aaron

On Wed, May 16, 2018 at 7:28 AM, Hans Wennborg via cfe-dev
<cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>> wrote:
> Sorry for the late reply. I didn't see the thread until now.
>
> On Sat, May 12, 2018 at 1:26 AM, Shoaib Meenai via cfe-dev
> <cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>> wrote:
>> Hi all,
>>
>>
>>
>> Currently, clang's format specifier check warnings (-Wformat) check for
>> exact type matches between the specifier and the corresponding argument. I'm
>> proposing adding an option to relax these warnings such that it doesn't warn
>> when there's a type mismatch but the types of the specifier and the argument
>> have the same size and alignment (and are both integral). For an example, on
>> an LLP64 system, attempting to print a long with "%d" would not warn, but
>> attempting to print a long long with "%d" would still warn. We could either
>> do this relaxation for -Wformat and add a new option for the stricter
>> warnings, or keep -Wformat as is and add a new option for the relaxed
>> warnings.
>>
>>
>>
>> The motivation is that during the 6.0 timeframe, clang gained format
>> specifier checking for %zd (r308067) and %tu (r314470), which made -Wformat
>> fire in a lot of additional cases. A particularly common case is programs
>> using %zd to print NSInteger. This will be warning-free on 64-bit targets,
>> where both ssize_t and NSInteger are long, but warn on 32-bit targets, where
>> ssize_t is still long but NSInteger is int. This additional warning
>> motivated https://reviews.llvm.org/D42933<https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D42933&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=Dc2LUIK-XaYupO2Hkkb0rUQ1P7J1i_d1FWGuq9xVIPo&s=TLCnZBAN4H1MpoWz2cq-3bvr7rxnMWTy5MkbjxlzalA&e=>, which attempted to special case
>> %z and NSInteger; the diff's comments have a bunch of additional discussion
>> about this issue. The conclusion there was that, although having mismatched
>> specifiers of this nature is definitely technically incorrect, there
>> shouldn't actually be any issues in practice when the size and alignment of
>> the specifier and the argument match. Ideally people would update their
>> codebases to use proper specifiers, but that can be impractical for large
>> codebases, and it would be really nice to have a way to restrict the warning
>> to the cases which will actually cause problems in practice.
>
> 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?
>
>> Note that -Wformat isn't meant to be a general portability checker, since it
>> only ever raises warnings when there's a type mismatch *on the current
>> target*, not if there's any possibility of a mismatch.
>
> 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.
>
>> For example, you
>> could be printing out a size_t using %lu, and you wouldn't get warnings if
>> your compilation target used unsigned long as the underlying type for
>> size_t, even though that underlying type could differ across targets.
>
> Right, because doing this kind of portability checks would be hard:
> the compiler can't know if the source used unsigned long and was lucky
> that it matched size_t, or if unsigned long had been carefully
> selected to match.
>
>> This
>> proposal does mean we would lose out on some additional portability
>> warnings; continuing the previous example, we would now not get warnings for
>> printing out a size_t using %lu even for a compilation target where size_t
>> was actually an unsigned int, assuming that the target had the same size and
>> alignment for int and long (as most 32-bit platforms do). However, given
>> that -Wformat was never meant to be a general portability checker, and that
>> there would still be an option to get the fully strict warnings, I don't
>> think this is too much of an issue in practice.
>
> 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.
>
>> What are people's thoughts here? Are there preferences for relaxing -Wformat
>> and adding a new option for strict format warnings vs. keeping -Wformat
>> as-is and adding a new option for relaxed format warnings? Of course, we
>> should also ensure that the optimizer doesn't do anything surprising when
>> there's a type mismatch between the specifier and the argument but both
>> types have the same size and alignment (i.e., the case where the relaxed
>> format warning won't fire), both now and in the future.
>
> 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.
>
> I'm not opposed to adding a -Wformat-relaxed option if there's a lot
> of demand for it.
>
> TL;DR: What Joerg said.
>
> Thanks,
> Hans
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_cfe-2Ddev&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=Dc2LUIK-XaYupO2Hkkb0rUQ1P7J1i_d1FWGuq9xVIPo&s=235uPgu-k8-11nIEt_1Br4jxzCB_Oj0sZboRl8d5uwo&e=>
_______________________________________________
cfe-dev mailing list
cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_cfe-2Ddev&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=Dc2LUIK-XaYupO2Hkkb0rUQ1P7J1i_d1FWGuq9xVIPo&s=235uPgu-k8-11nIEt_1Br4jxzCB_Oj0sZboRl8d5uwo&e=>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180516/85ae183e/attachment.html>