[cfe-dev] Relaxing format specifier checks

[Aaron Ballman via cfe-dev]

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> 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> 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, 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
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev