[cfe-dev] Relaxing format specifier checks

JF Bastien via cfe-dev cfe-dev at lists.llvm.org
Thu May 17 09:12:50 PDT 2018 

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

In the same way, Shoaib is representing Facebook’s experience. I don’t have the same experience, I’m nonetheless sympathetic to the point being made because it comes from a different codebase. I’m happy to let Shoaib speak up for that experience, and propose solutions which make sense for them.


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

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

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.


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

Yes please :-)


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


Quoting John again:

> 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 <https://reviews.llvm.org/D42933#1091502>


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

Indeed, this proposal is broader. D42933 suggest a very specific fix for NSInteger and %z formats on Darwin platforms. Shoaib suggested we broaden the fix based on Facebook’s experience with portability warnings. I’m happy to broaden the fix if it addresses the original problem. If we pursue something which doesn’t address the original problem then I want to pursue a separate fix for that problem, and we’re back to the original suggestion in D42933.


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

No. The argument is that NSInteger with %z is guaranteed to always work on Darwin, therefore there is no portability problem on Darwin and the warning is a false-positive. False-positive warnings shouldn’t be on at -Wall, especially at the quantity of warnings it emits in user code (this one is particularly noisy).

There’s a separate argument on the futility of portability warnings, for that see the quote from John above.


>>> 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 hope my response above answered this point.


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

I think we agree here.


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

No, there are 2 distinct problems: one on Darwin, one on Facebook’s codebase. Shoaib suggested addressing both issues with one fix. We might decide to address both separately. Shoaib’s proposal is indeed further-reaching, hence the cfe-dev proposal.


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

2 problems on at least 2 platforms. We haven’t heard from other platforms, which doesn’t mean it’s restricted to 2 platforms. A warning working well in your experience is a good datapoint, but by no means is it the only valid datapoint.


> Cheers,
> Hans


> On May 17, 2018, at 8:01 AM, Hubert Tong <hubert.reinterpretcast at gmail.com> wrote:
> 
> On Thu, May 17, 2018 at 1:56 AM, JF Bastien <jfbastien at apple.com <mailto:jfbastien at apple.com>> wrote:
> 
> 
>> On May 16, 2018, at 10:51 PM, Hubert Tong <hubert.reinterpretcast at gmail.com <mailto:hubert.reinterpretcast at gmail.com>> wrote:
>> 
>> On Thu, May 17, 2018 at 1:36 AM, JF Bastien <jfbastien at apple.com <mailto:jfbastien at apple.com>> wrote:
>> 
>>> On May 16, 2018, at 10:15 PM, Hubert Tong via cfe-dev <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>>> 
>>> On Fri, May 11, 2018 at 7:26 PM, Shoaib Meenai via cfe-dev <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>>> 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.
>>> The "contract" for the format string and the corresponding argument is as described elsewhere in the thread, and it is rather more interesting for scanf than not. GCC's implementation technology is apparently able to optimize (at -O2 and up, at least when targeting powerpc64le-linux-gnu) the check feeding the abort() in the following to not observe the write performed in the ersatz scanf():
>>> #include <stdarg.h>
>>> void abort(void);
>>> int strcmp(const char *, const char *);
>>> 
>>> typedef int Type;
>>> typedef long NType;
>>> 
>>> int myvscanf(const char *const fmt, va_list ap) {
>>>   if (strcmp(fmt, "%ld") != 0) { return 0; }
>>> 
>>>   NType *const p = va_arg(ap, NType *);
>>>   *p = 42;
>>>   return 1;
>>> }
>>> 
>>> __attribute__((__format__(__scanf__, 2, 3)))
>>> __attribute__((__noinline__, __noclone__))
>>> void updateStatusFromPipe(Type *statusp, const char *const fmt, ...) {
>>>   va_list ap;
>>>   va_start(ap, fmt);
>>>   int ret = 0;
>>>   if (*statusp == 0) {
>>>     ret = myvscanf(fmt, ap);
>>>   }
>>>   va_end(ap);
>>>   if (ret >= 1 && *statusp != 0) abort();
>>> }
>>> 
>>> int main(void) {
>>>   Type status = 0;
>>>   updateStatusFromPipe(&status, "%ld", &status);
>>> } 
>>> 
>>> The "now or in the future" sounds like needing to do "worse" or "better" than what GCC is doing to avoid "anything surprising", because "on par" with GCC fits the bill for something "surprising". Which is to say that, in my opinion, there is no avoiding "something surprising" "in the future" on code that the proposed relaxation is letting slip through.
>>> 
>>> TL;DR: Please don't change the default.
>> 
>> The default has changed recently. We’re making the case that parts should be relegated to their own flag.
>> 
>> Your argument ignores the prerogative that platforms have in providing additional guarantees. NSInteger has that guarantee on Darwin platforms, it should be honored. I further don’t see how optimizing printf as you seem to suggest could be possible is ever useful or desirable.
>> Why is changing the behaviour for "everyone" for possible guarantees on some platforms the approach as opposed to adjusting the warning based on the guarantees that the target to known to provide?
> 
> I suspect that reading https://reviews.llvm.org/D42933 <https://reviews.llvm.org/D42933> would help understand the current proposal.
> Okay, I needed the context in terms of how the proposal evolved. Thanks.

Great! Sorry there’s a lot of context to ingest.


> If not, I’d like to understand which part of D42933 you don’t agree with.
> Something Darwin-specific was considered to begin with in D42933; however, the discussion has evolved, and the version being proposed in this thread applies past the default that "recently changed". I am responding to this proposal, not D42933 (which, at this point, is more a discussion than a proposal).

Sure. The reason I bring back D42933 is, as I’ve stated to Hans above, that if this proposal doesn’t address D42933 then we still need to address D42933. Further, D42933 is relevant because it’s a fairly similar datapoint to Facebook’s, albeit much more narrow.


> Specifically, is it about platform guarantees,
> I understand that platforms may provide guarantees, and that software may be written with those platforms in mind. Clang might not be limited to platforms with certain specific guarantees, and so, I think that adjusting for platform guarantees should be opt-in (by the target or otherwise).

Opt-in by the platform was the original D42933 proposal. It sounds like you’re OK with that approach if Shoaib’s proposal goes in another direction?


> Furthermore, the extent of the specific guarantee being mentioned might be of interest. Is this a platform guarantee beyond having the same object representation, same value representation for all values, and the same alignment? A possible reading of the proposal with regards to conversion specifications for pointers to integral types would say that the proposal is talking about the type pointed to. Guarantees on the validity of using glvalues of one type to access objects of the other would then be of interest.
> 
> I agree that it is possible to read the proposal as not applying to pointer arguments. I am hoping we are good with clarifying on taking the latter interpretation.

This is one of the reasons I dislike -Wformat-relaxed as a name. Relaxed what?


> portability warnings,
> The proposed change goes beyond nixing portability checking. It is a shift in a contract between the caller providing a format string, and a callee processing a format string. It assumes specific details about how variable arguments work in the calling convention, how the attendant macros are implemented, and limitations in optimizer technology. It may be weird, but not prohibited, for types that cannot be differentiated when in memory to have different treatment in the registers used to pass them.

I don’t agree with you here, and defer to John’s comment (quoted above) for why.


> printf specifically versus other format-like warnings, etc?
> I think the major use case for the proposal is covered by limiting the scope to changing the checking of arguments of integral type to the "system-provided" printf family of functions.
> 
> 
> 
>>> _______________________________________________
>>> 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 <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180517/2319b298/attachment.html>

More information about the cfe-dev mailing list