|From:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|Subject:||Allowing printf("%m") only where it actually works|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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 , which was
soon followed by some well-reasoned push-back ; 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 , 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 . 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 , 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
|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|