[v2,0/2] C99: remove hardcoded-out !HAVE_VARIADIC_MACROS code

Message

Ævar Arnfjörð Bjarmason Jan. 28, 2022, 11:11 a.m. UTC

This v2 of $subject is being submitted on the one-year anniversary of
the patch to hardcode away the !HAVE_VARIADIC_MACROS code.

This is another small step on the way towards changing the usage.[ch]
API so that we can log the actual file/line number of
usage()/warning()/error()/die() in trace2 events. See a greater RFC
series to do that at:
https://lore.kernel.org/git/[e-mail]/

This should address all feedback raised on v1, including Junio
thinking it was a no-go since the submission in April 2021 was was
relatively recently after we'd hardcoded away
!HAVE_VARIADIC_MACROS. We've now had several releases with that code,
and other recent C99 test balloons since then.

The since of the range-diff is mainly because in v1 I didn't notice
that I needed to move the relevant API docs around, which this v2
does.

Ævar Arnfjörð Bjarmason (2):
  git-compat-util.h: clarify GCC v.s. C99-specific in comment
  C99: remove hardcoded-out !HAVE_VARIADIC_MACROS code

 Documentation/CodingGuidelines |   3 +
 banned.h                       |   5 --
 git-compat-util.h              |  16 +---
 trace.c                        |  80 +-------------------
 trace.h                        | 133 +++++++++++++--------------------
 trace2.c                       |  39 ----------
 trace2.h                       |  25 -------
 usage.c                        |  15 +---
 8 files changed, 59 insertions(+), 257 deletions(-)

Range-diff against v1:
1:  a8cc05cf56f ! 1:  31079a71ecb git-compat-util.h: clarify comment on GCC-specific code
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## Commit message ##
    -    git-compat-util.h: clarify comment on GCC-specific code
    +    git-compat-util.h: clarify GCC v.s. C99-specific in comment
     
         Change a comment added in e208f9cc757 (make error()'s constant return
    -    value more visible, 2012-12-15) to note that the code doesn't only
    -    depend on variadic macros, which have been a hard dependency since
    -    765dc168882 (git-compat-util: always enable variadic macros,
    -    2021-01-28), but also on GCC's handling of __VA_ARGS__. The commit
    -    message for e208f9cc757 made this clear, but the comment it added did
    -    not.
    +    value more visible, 2012-12-15). It's not correct that this is GCC-ism
    +    anymore, it's code that uses standard C99 features.
    +
    +    The comment being changed here pre-dates the HAVE_VARIADIC_MACROS
    +    define, which we got in e05bed960d3 (trace: add 'file:line' to all
    +    trace output, 2014-07-12).
    +
    +    The original implementation of an error() macro) in e208f9cc757 used a
    +    GCC-ism with the paste operator (see the commit message for mention of
    +    it), but that was dropped later by 9798f7e5f9 (Use __VA_ARGS__ for all
    +    of error's arguments, 2013-02-08), giving us the C99-portable version
    +    we have now.
    +
    +    While we could remove the __GNUC__ define here, it might cause issues
    +    for other compilers or static analysis systems, so let's not. See
    +    87fe5df365 (inline constant return from error() function, 2014-05-06)
    +    for one such issue.
     
         See also e05bed960d3 (trace: add 'file:line' to all trace output,
         2014-07-12) for another comment about GNUC's handling of __VA_ARGS__.
    @@ Commit message
     
      ## git-compat-util.h ##
     @@ git-compat-util.h: void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
    - 
      /*
       * Let callers be aware of the constant return value; this can help
    -- * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
    +  * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
     - * because some compilers may not support variadic macros. Since we're only
     - * trying to help gcc, anyway, it's OK; other compilers will fall back to
     - * using the function as usual.
    -+ * gcc with -Wuninitialized analysis.
    -+ *
    -+ * We restrict this trick to gcc, though, because while we rely on the
    -+ * presence of C99 variadic macros, this code also relies on the
    -+ * non-standard behavior of GCC's __VA_ARGS__, allowing error() to
    -+ * work even if no format specifiers are passed to error().
    -+ *
    -+ * Since we're only trying to help gcc, anyway, it's OK; other
    -+ * compilers will fall back to using the function as usual.
    ++ * because other compilers may be confused by this.
       */
      #if defined(__GNUC__)
      static inline int const_error(void)
2:  f12e3cad57d ! 2:  966d96505cb C99 support: remove non-HAVE_VARIADIC_MACROS code
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## Commit message ##
    -    C99 support: remove non-HAVE_VARIADIC_MACROS code
    +    C99: remove hardcoded-out !HAVE_VARIADIC_MACROS code
     
    -    Remove code that depend on HAVE_VARIADIC_MACROS not being set. Since
    -    765dc168882 (git-compat-util: always enable variadic macros,
    -    2021-01-28) we've unconditionally defined it to be true, and that
    -    change went out with v2.31.0. This should have given packagers enough
    -    time to discover whether variadic macros were an issue.
    +    Remove the "else" branches of the HAVE_VARIADIC_MACROS macro, which
    +    has been unconditionally omitted since 765dc168882 (git-compat-util:
    +    always enable variadic macros, 2021-01-28).
     
    -    It seems that they weren't, so let's update the coding guidelines and
    -    remove all the fallback code for the non-HAVE_VARIADIC_MACROS case.
    +    Since they were hardcoded out anyone trying to compile a version of
    +    git v2.31.0 or later would have had a compilation error. 10 months
    +    across a few releases since then should have been enough time for
    +    anyone who cared to run into that and report the issue.
    +
    +    In addition to that, for anyone unsetting HAVE_VARIADIC_MACROS we've
    +    been emitting extremely verbose warnings since at least
    +    ee4512ed481 (trace2: create new combined trace facility,
    +    2019-02-22). That's because there is no such thing as a
    +    "region_enter_printf" or "region_leave_printf" format, so at least
    +    under GCC and Clang everything that includes trace.h (almost every
    +    file) emits a couple of warnings about that.
    +
    +    There's a large benefit to being able to have a hard dependency rely
    +    on variadic macros, the code surrounding usage.c is hard to maintain
    +    if we need to write two implementations of everything, and by relying
    +    on "__FILE__" and "__LINE__" along with "__VA_ARGS__" we can in the
    +    future make error(), die() etc. log where they were called from. We've
    +    also recently merged d67fc4bf0ba (Merge branch 'bc/require-c99',
    +    2021-12-10) which further cements our hard dependency on C99.
    +
    +    So let's delete the fallback code, and update our CodingGuidelines to
    +    note that we depend on this. The added bullet-point starts with
    +    lower-case for consistency with other bullet-points in that section.
    +
    +    The diff in "trace.h" is relatively hard to read, since we need to
    +    retain the existing API docs, which were comments on the code used if
    +    HAVE_VARIADIC_MACROS was not defined.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
    @@ git-compat-util.h: static inline int regexec_buf(const regex_t *preg, const char
     
      ## trace.c ##
     @@ trace.c: static int prepare_trace_line(const char *file, int line,
    - 	strbuf_addf(buf, "%02d:%02d:%02d.%06ld ", tm.tm_hour, tm.tm_min,
    - 		    tm.tm_sec, (long) tv.tv_usec);
    - 
    + 	gettimeofday(&tv, NULL);
    + 	secs = tv.tv_sec;
    + 	localtime_r(&secs, &tm);
    +-	strbuf_addf(buf, "%02d:%02d:%02d.%06ld ", tm.tm_hour, tm.tm_min,
    +-		    tm.tm_sec, (long) tv.tv_usec);
    +-
     -#ifdef HAVE_VARIADIC_MACROS
    - 	/* print file:line */
    - 	strbuf_addf(buf, "%s:%d ", file, line);
    +-	/* print file:line */
    +-	strbuf_addf(buf, "%s:%d ", file, line);
    ++	strbuf_addf(buf, "%02d:%02d:%02d.%06ld %s:%d", tm.tm_hour, tm.tm_min,
    ++		    tm.tm_sec, (long) tv.tv_usec, file, line);
      	/* align trace output (column 40 catches most files names in git) */
      	while (buf->len < 40)
      		strbuf_addch(buf, ' ');
    @@ trace.h: void trace_command_performance(const char **argv);
     - */
     -void trace_strbuf(struct trace_key *key, const struct strbuf *data);
     -
    --/**
    + /**
     - * Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled.
    -- *
    ++ * Macros to add the file:line of the calling code, instead of that of
    ++ * the trace function itself.
    +  *
     - * Example:
     - * ------------
     - * uint64_t t = 0;
    @@ trace.h: void trace_command_performance(const char **argv);
     -
     -#else
     -
    - /*
    -  * Macros to add file:line - see above for C-style declarations of how these
    -  * should be used.
    +-/*
    +- * Macros to add file:line - see above for C-style declarations of how these
    +- * should be used.
    +- */
    +-
    +-/*
    +- * TRACE_CONTEXT may be set to __FUNCTION__ if the compiler supports it. The
    +- * default is __FILE__, as it is consistent with assert(), and static function
    +- * names are not necessarily unique.
    +- *
    +- * __FILE__ ":" __FUNCTION__ doesn't work with GNUC, as __FILE__ is supplied
    +- * by the preprocessor as a string literal, and __FUNCTION__ is filled in by
    +- * the compiler as a string constant.
    +- */
    +-#ifndef TRACE_CONTEXT
    +-# define TRACE_CONTEXT __FILE__
    +-#endif
    +-
    +-/*
    +  * Note: with C99 variadic macros, __VA_ARGS__ must include the last fixed
    +  * parameter ('format' in this case). Otherwise, a call without variable
    +  * arguments will have a surplus ','. E.g.:
    +@@ trace.h: void trace_performance_leave(const char *format, ...);
    +  *
    +  * which is invalid (note the ',)'). With GNUC, '##__VA_ARGS__' drops the
    +  * comma, but this is non-standard.
    ++ *
    ++ * TRACE_CONTEXT may be set to __FUNCTION__ if the compiler supports it. The
    ++ * default is __FILE__, as it is consistent with assert(), and static function
    ++ * names are not necessarily unique.
    ++ *
    ++ * __FILE__ ":" __FUNCTION__ doesn't work with GNUC, as __FILE__ is supplied
    ++ * by the preprocessor as a string literal, and __FUNCTION__ is filled in by
    ++ * the compiler as a string constant.
    ++ */
    ++#ifndef TRACE_CONTEXT
    ++# define TRACE_CONTEXT __FILE__
    ++#endif
    ++
    ++/**
    ++ * Prints a formatted message, similar to printf.
    +  */
    ++#define trace_printf(...) trace_printf_key(&trace_default_key, __VA_ARGS__)
    + 
    + #define trace_printf_key(key, ...)					    \
    + 	do {								    \
    +@@ trace.h: void trace_performance_leave(const char *format, ...);
    + 					    __VA_ARGS__);		    \
    + 	} while (0)
    + 
    +-#define trace_printf(...) trace_printf_key(&trace_default_key, __VA_ARGS__)
    +-
    ++/**
    ++ * Prints a formatted message, followed by a quoted list of arguments.
    ++ */
    + #define trace_argv_printf(argv, ...)					    \
    + 	do {								    \
    + 		if (trace_pass_fl(&trace_default_key))			    \
    +@@ trace.h: void trace_performance_leave(const char *format, ...);
    + 					    argv, __VA_ARGS__);		    \
    + 	} while (0)
    + 
    ++/**
    ++ * Prints the strbuf, without additional formatting (i.e. doesn't
    ++ * choke on `%` or even `\0`).
    ++ */
    + #define trace_strbuf(key, data)						    \
    + 	do {								    \
    + 		if (trace_pass_fl(key))					    \
    + 			trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
    + 	} while (0)
    + 
    ++/**
    ++ * Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled.
    ++ *
    ++ * Example:
    ++ * ------------
    ++ * uint64_t t = 0;
    ++ * for (;;) {
    ++ * 	// ignore
    ++ * t -= getnanotime();
    ++ * // code section to measure
    ++ * t += getnanotime();
    ++ * // ignore
    ++ * }
    ++ * trace_performance(t, "frotz");
    ++ * ------------
    ++ */
    + #define trace_performance(nanos, ...)					    \
    + 	do {								    \
    + 		if (trace_pass_fl(&trace_perf_key))			    \
    +@@ trace.h: void trace_performance_leave(const char *format, ...);
    + 					     __VA_ARGS__);		    \
    + 	} while (0)
    + 
    ++/**
    ++ * Prints elapsed time since 'start' if GIT_TRACE_PERFORMANCE is enabled.
    ++ *
    ++ * Example:
    ++ * ------------
    ++ * uint64_t start = getnanotime();
    ++ * // code section to measure
    ++ * trace_performance_since(start, "foobar");
    ++ * ------------
    ++ */
    + #define trace_performance_since(start, ...)				    \
    + 	do {								    \
    + 		if (trace_pass_fl(&trace_perf_key))			    \
    +@@ trace.h: void trace_performance_leave(const char *format, ...);
    + 						   __VA_ARGS__);	    \
    + 	} while (0)
    + 
    ++
    + /* backend functions, use non-*fl macros instead */
    + __attribute__((format (printf, 4, 5)))
    + void trace_printf_key_fl(const char *file, int line, struct trace_key *key,
     @@ trace.h: static inline int trace_pass_fl(struct trace_key *key)
      	return key->fd || !key->initialized;
      }
    @@ trace2.h: void trace2_printf_va_fl(const char *file, int line, const char *fmt,
       * Optional platform-specific code to dump information about the
     
      ## usage.c ##
    +@@ usage.c: static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
    + 	va_copy(params_copy, params);
    + 
    + 	/* truncation via snprintf is OK here */
    +-	if (file)
    +-		snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
    +-	else
    +-		snprintf(prefix, sizeof(prefix), "BUG: ");
    ++	snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
    + 
    + 	vreportf(prefix, fmt, params);
    + 
     @@ usage.c: static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
      	abort();
      }