[cfe-dev] Relaxing format specifier checks

[JF Bastien via cfe-dev]

> On May 17, 2018, at 11:43 PM, Hans Wennborg <hans at chromium.org> wrote:
> 
> 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.

Significant enough that I’m spending the time to change clang.


>> 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 not sure I get your point of view. Is it that you don’t think platforms can offer guarantees beyond what the language mandates; you don’t think Darwin offers said guarantees; you think the guarantee Darwin offers is bogus; or you think that in instances where platforms offer these supplemental guarantees the compiler should warn regardless? Or is it something else?


>> 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 need to understand the answer to my question above before we can really discuss this, but basically my POV is that a warning about something a platform guarantees will never be an issue shouldn’t be on by default at -Wall, especially if it’s been show to be quite noisy in real-world code. You can opt-in to more warnings with -Wpedantic or other sub-pedantic flags, but you shouldn’t get it out of -Wall.


>>> 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?

Three people working on maintaining the Darwin platform have said that they believe the direction of D42933 is good, and we’ve discussed with others internally. I’m not sure what’s the root cause of our disagreement, but I’d like to offer you sufficient data to at least believe that this has been thought about.


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

Sure, warnings should hold code to a high standard, but only when it’s useful churn. D42933 suggests it is not in this case. I want to stay on topic, but if you want we can separately play the “useless hypothetical warnings holding code to a higher standard” game, I bet I can come up with fun ones :-)


> 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