Relaxing format specifier checks

Clang Frontend
#1

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.

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

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.

Thanks,

Shoaib

#2

Thanks for writing this up!

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.

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

What are people's thoughts here? Are there preferences for relaxing -Wformat and adding a new option for strict format warnings

I much prefer this approach. We know that our portability warnings are a best-effort thing, and it would be best for developers to opt-in to them. I don’t thing -Wformat-portability should be on by default unless we also do things as suggested in D42933 (where we know that size and alignment of NSInteger / NSUInteger always match that of size_t / ssize_t, so there’s no actual portability issue).

vs. keeping -Wformat as-is and adding a new option for relaxed format warnings?

I don’t like this approach. I think having -Wformat-relaxed / -Wformat / -Wformat-pedantic is confusing.

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.

+1

#3

Add a new relaxed mode, don't just relax the existing checks. We've had
a long history of the GCC printf checks being less useful than possible
exactly because it cares only about the promoted (effective) type.

Joerg

#4

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?

+1 to change the default behavior and put the stricter checks in another flag, for all the reasons discussed already.

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 think the concern here would be if clang did calling convention lowering by generating a store of the outgoing argument with tbaa attached, and then also generated a (potentially mismatched) tbaa-annotated load for the call to va_arg inside the printf function. And then optimized the two functions together.

I don’t believe that can happen, both because the call arguments are not lowered to memory stores (if needed) by clang, but rather by llvm (there’s no real possibility to attach TBAA at that point, even if we wanted to), and also because clang doesn’t attach tbaa to the loads generated by va_arg.

#5

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?

+1 to change the default behavior and put the stricter checks in another flag, for all the reasons discussed already.

Sounds like there’s support for doing this.

Shoaib, how would you rather proceed?

#6

I was planning on waiting a few more days to give more people a chance to chime in. Let’s wait till the end of the week?

#7

Sorry for the late reply. I didn't see the thread until now.

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

#8

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

#9

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)

#10

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.

#11

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.

Correct.

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.

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@lists.llvm.org> on behalf of cfe-dev <cfe-dev@lists.llvm.org>
Reply-To: Nico Weber <thakis@chromium.org>
Date: Wednesday, May 16, 2018 at 5:33 AM
To: Aaron Ballman <aaron@aaronballman.com>
Cc: cfe-dev <cfe-dev@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:

That’s not a solution that should be recommended users go with, and it’s not something most users would go with. It therefore doesn’t fix the issue.

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

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

> Sorry for the late reply. I didn't see the thread until now.
>
>> 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?

NSInteger is, on purpose, the same size as a size_t. The types can’t change.

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

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.

We can be standards pedants, or we can accept that this case is actually OK according to the platform.

I could also get the standard changed, but that’ll take a while and I’m not sure it’s worthwhile.

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

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

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

We’d be relaxing something that only just got added to clang, and which tries to catch portability issues. 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.

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

#12

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.

#13

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.

#14

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?

#15

I suspect that reading https://reviews.llvm.org/D42933 would help understand the current proposal. If not, I’d like to understand which part of D42933 you don’t agree with. Specifically, is it about platform guarantees, portability warnings, printf specifically versus other format-like warnings, etc?

#16

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.

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.

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

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?

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.

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.

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

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.

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.

Cheers,
Hans

#17

This is a convincing point, but the reasoning here is somewhat different, since it’s dealing with pointer-types, and then an actual pointer-dereference of the incorrect type. That’s certainly a bad idea – I agree it’d be quite unwise to change the behavior when dealing with pointers, either in the scanf specifiers or for the expected pointee-type of %n in printf.

I think the question at hand is whether a type-mismatch of the value itself is a problem you should be worried about. E.g., calling “va_arg(long)” when passing a value of type int to the call, where int and long are “the same”.

#18

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 would help
understand the current proposal.

Okay, I needed the context in terms of how the proposal evolved. Thanks.

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

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

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.

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.

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.

#19

This is a convincing point, but the reasoning here is somewhat different,
since it's dealing with pointer-types, and then an actual
pointer-dereference of the incorrect type. That's certainly a bad idea -- I
agree it'd be quite unwise to change the behavior when dealing with
pointers, either in the scanf specifiers or for the expected pointee-type
of %n in printf.

I think the question at hand is whether a type-mismatch of the value
itself is a problem you should be worried about. E.g., calling
"va_arg(long)" when passing a value of type int to the call, where int and
long are "the same".

So, platform-specific knowledge can cut both ways in this space. Suppose
that on some platform, the ellipsis on a function simply becomes an extra void
* argument, the space for the variable arguments are created as a local
structure with the arguments (with no packing or surprising alignment
rules), and va_arg is implemented to cast to a pointer to the type provided
after adjusting the current pointer in the va_list. Modifying the Clang AST
to make these aspects explicit will create the TBAA information. Assuming
that on this platform the void */char * compatibility under va_arg is taken
into account by TBAA, the modification of the Clang AST should be safe.
Maybe the platform guarantees that users can do so at the source level. The
question becomes one of how "int" and "long" should be determined to be
"the same" or not. It looks like a property of the target to me. My other
note mentions that it is possible for different registers to be used for
passing types that are indistinguishable when in memory.

#20

(Responding to Hans first, then Hubert below)

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 :slight_smile:

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

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

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

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.

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?

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

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