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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Allowing printf("%m") only where it actually works
Date: 2018-05-21 00:30:05
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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=]

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.


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


Attachment Content-Type Size
get-compiler-warnings-for-misuse-of-percent-m-1.patch text/x-diff 6.7 KB


Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Gierth 2018-05-21 00:38:36 Re: Fix for FETCH FIRST syntax problems
Previous Message Carter Thaxton 2018-05-20 23:48:44 Add --include-table-data-where option to pg_dump, to export only a subset of table data