Re: On non-Windows, hard depend on uselocale(3)

Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Nov 16, 2023 at 12:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Thomas Munro <thomas.munro@gmail.com> writes:
>>> Perhaps we could use snprintf_l() and strtod_l() where available.
>>> They're not standard, but they are obvious extensions that NetBSD and
>>> Windows have, and those are the two systems for which we are doing
>>> grotty things in that code.

>> Yeah.  I'd say the _l functions should be preferred over uselocale()
>> if available, but sadly they're not there on common systems.  (It
>> looks like glibc has strtod_l but not snprintf_l, which is odd.)

> Here is a first attempt.

I've not reviewed this closely, but I did try it on mamba's host.
It compiles and passes regression testing, but I see two warnings:

common.c: In function 'PGTYPESsprintf':
common.c:120:2: warning: function 'PGTYPESsprintf' might be a candidate for 'gnu_printf' format attribute
[-Wsuggest-attribute=format]
  120 |  return vsprintf_l(str, PGTYPESclocale, format, args);
      |  ^~~~~~
common.c: In function 'PGTYPESsnprintf':
common.c:136:2: warning: function 'PGTYPESsnprintf' might be a candidate for 'gnu_printf' format attribute
[-Wsuggest-attribute=format]
  136 |  return vsnprintf_l(str, size, PGTYPESclocale, format, args);
      |  ^~~~~~

That happens because on NetBSD, we define PG_PRINTF_ATTRIBUTE as
"__syslog__" so that the compiler will not warn about use of %m
(apparently, they support %m in syslog() but not printf(), sigh).

I think this is telling us about an actual problem: these new
functions are based on libc's printf not what we have in snprintf.c,
and therefore we really shouldn't be assuming that they will support
any format specs beyond what POSIX requires for printf.  If somebody
tried to use %m in one of these calls, we'd like to get warnings about
that.

I experimented with the attached delta patch and it does silence
these warnings.  I suspect that ecpg_log() should be marked as
pg_attribute_std_printf() too, because it has the same issue,
but I didn't try that.  (Probably, we see no warning for that
because the compiler isn't quite bright enough to connect the
format argument with the string that gets passed to vfprintf().)

            regards, tom lane

diff --git a/src/include/c.h b/src/include/c.h
index 82f8e9d4c7..98e3bbf386 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -171,13 +171,19 @@
 #define PG_USED_FOR_ASSERTS_ONLY pg_attribute_unused()
 #endif

-/* GCC and XLC support format attributes */
+/*
+ * GCC and XLC support format attributes.  Use pg_attribute_printf()
+ * for our src/port/snprintf.c implementation and functions based on it.
+ * Use pg_attribute_std_printf() for functions based on libc's printf.
+ */
 #if defined(__GNUC__) || defined(__IBMC__)
 #define pg_attribute_format_arg(a) __attribute__((format_arg(a)))
 #define pg_attribute_printf(f,a) __attribute__((format(PG_PRINTF_ATTRIBUTE, f, a)))
+#define pg_attribute_std_printf(f,a) __attribute__((format(printf, f, a)))
 #else
 #define pg_attribute_format_arg(a)
 #define pg_attribute_printf(f,a)
+#define pg_attribute_std_printf(f,a)
 #endif

 /* GCC, Sunpro and XLC support aligned, packed and noreturn */
diff --git a/src/interfaces/ecpg/include/pgtypes_format.h b/src/interfaces/ecpg/include/pgtypes_format.h
index d6dd06d361..87160fab59 100644
--- a/src/interfaces/ecpg/include/pgtypes_format.h
+++ b/src/interfaces/ecpg/include/pgtypes_format.h
@@ -20,7 +20,7 @@ extern int PGTYPESbegin_clocale(locale_t *old_locale);
 extern void PGTYPESend_clocale(locale_t old_locale);

 extern double PGTYPESstrtod(const char *str, char **endptr);
-extern int PGTYPESsprintf(char *str, const char *format, ...) pg_attribute_printf(2, 3);
-extern int PGTYPESsnprintf(char *str, size_t size, const char *format, ...) pg_attribute_printf(3, 4);
+extern int PGTYPESsprintf(char *str, const char *format, ...) pg_attribute_std_printf(2, 3);
+extern int PGTYPESsnprintf(char *str, size_t size, const char *format, ...) pg_attribute_std_printf(3, 4);

 #endif