[cfe-dev] Relaxing format specifier checks

[Hans Wennborg via cfe-dev]

On Thu, May 17, 2018 at 6:12 PM, JF Bastien via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
> (Responding to Hans first, then Hubert below)
>
> On May 17, 2018, at 3:58 AM, Hans Wennborg <hans at chromium.org> wrote:
>
> 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.
>
>
> I don’t understand where you want to go with this. Your and Google’s
> experience are relevant, but maintainers of the Darwin platform should be
> expected to hear much more about that platform, from a variety of codebase
> “flavors”. Google’s codebase, while very large, tends to be fairly uniform.
> Lack of data on your part can’t be extrapolated.

>From our end, it didn't seem like the world broke when %zi analysis
was added. My point was just that I'd like to understand better how
big this problem is.


> It sounds like a -Wformat-relaxed variant would serve this approach.
>
>
> This indeed helps Facebook’s codebase, and I’m happy for this outcome.
> However, it doesn’t resolve the original problem raised in D42933. If folks
> want to go with two solutions it’s OK. The original email was trying to
> solve both issues at once.

I still maintain that the warning discussed in D42933 is correct. The
problem to be solved is how to "relax" the warning for those who don't
think their code needs to be checked that strictly.

> I’m still non-plussed by the proposed name, though. “Relaxed” is awfully
> vague, when what’s being relaxed are specific portability warnings.

I don't feel strongly about the name either.

>
> I’d also like to point out John’s response from D42933:
>
>>>! In D42933#1092048, @rjmccall wrote:
>> I agree that the format-specifier checker is not intended to be a
>> portability checker.
>>
>> Any attempt to check portability problems has to account for two things:
>>   - Not all code is intended to be portable.  If you're going to warn
>> about portability problems, you need some way to not annoy programmers who
>> might have good reason to say that they only care about their code running
>> on Linux / macOS / 64-bit / 32-bit / whatever.  Generally this means
>> splitting the portability warning out as a separate warning group.
>>   - There are always established idioms for solving portability problems.
>> In this case, a certain set of important typedefs from the C standard have
>> been blessed with dedicated format modifiers like "z", but every other
>> typedef in the world has to find some other solution, either by asserting
>> that some existing format is good enough (as NSInteger does) or by
>> introducing a macro that expands to an appropriate format (as some things in
>> POSIX do).  A proper format-portability warning would have to know about all
>> of these conventions (in order to check that e.g. part of the format string
>> came from the right macro), which just seems wildly out-of-scope for a
>> compiler warning.
>
> I’m not sure -Wformat-relaxed is a good solution, but again if that’s what
> y’all want let’s do it, as well as something to resolve the original problem
> raised in D42933.

But the original problem is that Clang warns on printing NSInteger
with %zd, and having a -Wformat-relaxed or whatever would solve that
for users who don't want to fix their code. What else do you mean
needs fixing?

>> I'm not saying that the recommendation would change, but `long` being
>> pointer-sized is a pretty universal assumption on Darwin, I'm afraid.
>
>
> In the review I also said I’d update the documentation once this issue is
> resolved:
>
> https://reviews.llvm.org/D42933#1091502

Yes, but maybe that would be an update in the wrong direction? The
current documented advice of casting to long and printing with %zd
seems more correct maybe?


> 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”.

But shouldn't the warnings usually hold the code to a higher standard
than "does what's expected"?


Anyway, I'm not opposed to allowing the user to optionally relax this
aspect of the -Wformat warning. That would solve the problem for those
who want to print NSInteger with %zd, and more generally for those who
don't agree that warning about printing an int with %ld matters if
they're the same size. I don't care much about the name, but I think
that the "relaxed" behaviour should not be the default.

Cheers,
Hans