Horton

Allowing printf("%m") only where it actually works

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Date: 2018-05-21T00:30:05Z
Lists: pgsql-hackers

Attachments

<digression>

For amusement's sake, I was playing around with NetBSD-current (9-to-be)
today, and tried to compile Postgres on it.  It works OK --- and I can
even confirm that our new code for using ARM v8 CRC instructions works
there --- but I got a boatload of compile warnings like this:

latch.c:1180:4: warning: %m is only allowed in syslog(3) like functions [-Wformat=]
    ereport(ERROR,
    ^~~~~~~

A bit of googling turned up the patch that caused this [1], which was
soon followed by some well-reasoned push-back [2]; but the warning's
still there, so evidently the forces of bullheadedness won.  I was
ready to discount the whole thing as being another badly designed
no-wonder-gcc-upstream-won't-take-it compiler warning, when I noticed that
the last few warnings in my output were pointing out a live bug, to wit
using %m with plain old printf rather than elog/ereport.  So I fixed
that [3], but I'm thinking that we need to take a bit more care here.

</digression>

Looking around, we have circa seventy-five functions declared with
pg_attribute_printf in our tree right now, and of those, *only* the
elog/ereport support functions can be relied on to process %m correctly.
However, anyone who's accustomed to working in backend code is likely to
not think hard about using %m in an error message, as indeed the authors
and reviewers of pg_verify_checksums did not.  Worse yet, such cases
actually will work as long as you're testing on glibc platforms, only
to fail everywhere else.

So I think we need to try to make provisions for getting compiler warnings
when %m is used in a function that doesn't support it.  gcc on Linux-ish
platforms isn't going to be very helpful with this, but that doesn't mean
that we should confuse %m-supporting and not-%m-supporting functions,
as we do right now.

Hence, I think we need something roughly like the attached patch, which
arranges to use "gnu_printf" (if available) as the format archetype for
the elog/ereport functions, and plain "printf" for all the rest.  With
some additional hackery not included here, this can be ju-jitsu'd into
compiling warning-free on NetBSD-current.  (The basic idea is to extend
PGAC_C_PRINTF_ARCHETYPE so it will select __syslog__ as the archetype
if available; but then you need some hack to suppress the follow-on
warnings complained of in [2].  I haven't decided what's the least ugly
solution for the latter, so I'm not proposing such a patch yet.)

What I'm mainly concerned about at this stage is what effects this'll
have on Windows builds.  The existing comment for PGAC_C_PRINTF_ARCHETYPE
claims that using gnu_printf silences complaints about "%lld" and related
formats on Windows, but I wonder whether that is still true on Windows
versions we still support.  As I mentioned in [4], I don't think we really
work any longer on platforms that don't use "%lld" for "long long" values,
and it seems that Windows does accept that in post-XP versions --- but has
gcc gotten the word?

If this does work as desired on Windows, then that would be a fairly
mainstream platform that can produce warnings about wrong uses of %m,
even if gcc-on-Linux doesn't.  If worst comes to worst, somebody could
crank up a buildfarm machine with a newer NetBSD release.

Anyway, I don't feel a need to cram this into v11, since I just fixed
the live bugs of this ilk in HEAD; it seems like it can wait for v12.
So I'll add this to the next commitfest.

			regards, tom lane

[1] https://mail-index.netbsd.org/tech-userlevel/2015/08/21/msg009282.html
[2] https://mail-index.netbsd.org/tech-userlevel/2015/10/23/msg009371.html
[3] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=a13b47a59ffce6f3c13c8b777738a3aab1db10d3
[4] https://www.postgresql.org/message-id/13103.1526749980@sss.pgh.pa.us

Commits

  1. In pg_log_generic(), be more paranoid about preserving errno.

  2. Make src/common/exec.c's error logging less ugly.

  3. Select appropriate PG_PRINTF_ATTRIBUTE for recent NetBSD.

  4. Fix detection of the result type of strerror_r().

  5. Try another way to detect the result type of strerror_r().

  6. Clean up *printf macros to avoid conflict with format archetypes.

  7. Fix link failures due to snprintf/strerror changes.

  8. Implement %m in src/port/snprintf.c, and teach elog.c to rely on that.

  9. Always use our own versions of *printf().

  10. Incorporate strerror_r() into src/port/snprintf.c, too.

  11. Convert elog.c's useful_strerror() into a globally-used strerror wrapper.

  12. Revert "Distinguish printf-like functions that support %m from those that don't."

  13. Produce compiler errors if errno is referenced inside elog/ereport calls.

  14. Distinguish printf-like functions that support %m from those that don't.

  15. Fix unportable usage of printf("%m").

  16. Be more robust when strerror() doesn't give a useful result.