Re: [PATCH 4/6] *.h: add a few missing attribute((format))

From: Jeff King <peff@peff.net>
Date: Jul 12, 2021, 20:09 UTC
Message-ID: <YOyhd/odtQxwQk2W@coredump.intra.peff.net>
In-Reply-To: <877dhw4exj.fsf@evledraar.gmail.com>

On Mon, Jul 12, 2021 at 01:05:53AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 41 quoted lines

> So this:
> > [...]
> > For strbuf_addftime() let's add a strftime() format checker. Our
> > function understands the non-portable %z and %Z, see
> > c3fbf81a853 (strbuf: let strbuf_addftime handle %z and %Z itself,
> > 2017-06-15).
> >
> > That might be an issue in theory, but in practice we have existing
> > codepath that supplies a fixed string to strbuf_addftime(). We're
> > unlikely to run into the "%z" and "%Z" case at all, since it's used by
> > date.c and passed via e.g. "git log --date=<format>".
> > [...]
> >  /**
> > @@ -425,6 +426,7 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
> >   * `suppress_tz_name`, when set, expands %Z internally to the empty
> >   * string rather than passing it to `strftime`.
> >   */
> > +__attribute__((format (strftime, 2, 0)))
> >  void strbuf_addftime(struct strbuf *sb, const char *fmt,
> >  		    const struct tm *tm, int tz_offset,
> >  		    int suppress_tz_name);
> 
> Fails with compat/mingw.[ch] doing:
> 
>     [...]
>     compat/mingw.c:#undef strftime
>     compat/mingw.c-size_t mingw_strftime(char *s, size_t max,
>     compat/mingw.c-               const char *format, const struct tm *tm)
>     [...]
>     compat/mingw.h:#define strftime mingw_strftime
>     [...]
> 
> What's a good macro idiom to dig oneself out of that hole? The only
> similar thing I could find was NORETURN in git-compat-util.h being
> defined differently on various platforms, and then things like:
> 
> 	+#if !defined(__MINGW32__) && !defined(_MSC_VER)
> 	 __attribute__((format (strftime, 2, 0)))
> 	+#endif
> 
> Which seems to work, and may be the simplest workaround...

I don't think there's a general standard-C way to override the macro temporarily. You could perhaps fix it with re-ordering, but it is super-awkward in this case (mingw stuff clearly should go in git-compat-util.h or its includes, and the strbuf declaration should come after that).

Gcc at least supports __strftime__ in the attribute, so that could be an option (I've no clue how portable that is, though).

I'm skeptical on the utility of this strftime format checking in the first place, though. The printf one is matching up the format string with the otherwise-unchecked variable-length parameter list. That's very easy to get wrong, very bad if we do get it wrong, and very hard for anybody but the compiler to check accurately.

But a typo in the strftime placeholders is really no more interesting than a typo in any other error message. If it helps even some obscure cases it might be worth it, but:

  - we almost never feed a string literal anyway (and unlike printf,
    feeding non-literals is not dangerous). And in the one literal case
    you touched in the next patch, we had to make the code uglier to
    enable the checking.

  - as you note, this function isn't even a true strftime() replacement
    anyway. We allow %z, and I would not be at all opposed to adding
    more custom bits (either something regular strftime doesn't support
    at all, or extensions that are not universally available and we have
    to provide a fallback for).

  - it creates the macro headache you saw

  - I'm not sure if it creates other portability concerns. Does
    everybody who supports the format attribute support the strftime
    archetype? I have no clue.

So it doesn't really seem to be to be worth it on balance.

-Peff

Re: [PATCH 4/6] *.h: add a few missing __attribute__((format))

Re: [PATCH 4/6] *.h: add a few missing attribute((format))