About those snprintf invocation macros - Mailing list pgsql-hackers

Commit 8ecdefc26 reminded me of something I'd intended to whine about
and forgot.  port.h has this in its USE_REPL_SNPRINTF stanza:

/*
 *    The GCC-specific code below prevents the pg_attribute_printf above from
 *    being replaced, and this is required because gcc doesn't know anything
 *    about pg_printf.
 */
#ifdef __GNUC__
#define vsnprintf(...)  pg_vsnprintf(__VA_ARGS__)
#define snprintf(...)   pg_snprintf(__VA_ARGS__)
#define sprintf(...)    pg_sprintf(__VA_ARGS__)
#define vfprintf(...)   pg_vfprintf(__VA_ARGS__)
#define fprintf(...)    pg_fprintf(__VA_ARGS__)
#define printf(...)     pg_printf(__VA_ARGS__)
#else
#define vsnprintf       pg_vsnprintf
#define snprintf        pg_snprintf
#define sprintf         pg_sprintf
#define vfprintf        pg_vfprintf
#define fprintf         pg_fprintf
#define printf          pg_printf
#endif

In the first place, it seems like now that we're requiring C99 (and hence
__VA_ARGS__), we could drop the conditional.  However, there's more here
than meets the eye.  That comment doesn't explain what it's worried about
very well, but it's this: if we have defined PG_PRINTF_ATTRIBUTE as just
"printf" then writing "#define printf pg_printf" would break later uses of
pg_attribute_printf, since "pg_printf" is certainly not a known printf
archetype.

(BTW, if you're wondering whether this can still happen at all, yes it
can: the "gnu_printf" archetype seems to be a pretty recent invention,
because there are lots of gcc-using buildfarm animals that still pick
"printf" as the archetype.  Moreover, there are lots of machines with
system headers containing format(printf) attributes.  So we can't just
dismiss this hazard as not being a hazard anymore.)

AFAICS, though, that problem concerns only printf itself and not any
of its siblings.

Moreover, there's a reason *not* to use the (__VA_ARGS__) construction
except where we have to.  Precisely because the macros with that usage
don't expand for references that lack parens, we'd fail to replace
attempts to take a pointer to printf or one of its siblings.  So if
you did something like

    my_printf_hook = vsnprintf;

you would silently get a pointer to the system's vsnprintf not ours.
Right now this is a hazard only on Windows builds using GCC (which is bad
enough), but it'd become a hazard everywhere with my proposal to start
using our snprintf.c everywhere.

Fortunately, it doesn't seem very likely that people would want such
pointers to printf per se.  fprintf, or vsnprintf, say, seem like
much more likely candidates to be targets of function pointers.

So I propose that what we should do here is replace the above-quoted
fragment with

/*
 * We use __VA_ARGS__ for printf to prevent replacing references to
 * the "printf" format archetype in format() attribute declarations.
 * That unfortunately means that taking a function pointer to printf
 * will not do what we'd wish.  (If you need to do that, you must name
 * pg_printf explicitly.)  For printf's sibling functions, use
 * parameterless macros so that function pointers will work unsurprisingly.
 */
#define vsnprintf       pg_vsnprintf
#define snprintf        pg_snprintf
#define sprintf         pg_sprintf
#define vfprintf        pg_vfprintf
#define fprintf         pg_fprintf
#define printf(...)     pg_printf(__VA_ARGS__)

and make attendant simplifications in the plperl and plpython headers
that concern themselves with these macros.

I've not tested this, but does anyone see a flaw in the reasoning?

            regards, tom lane