Thread: Synchronize with imath upstream
pgcrypto bundles a copy of the imath library for arbitrary-precision integer
arithmetic in non-SSL builds. Upstream fixed buffer overflows through the
years, and commit 8b59672 brought those fixes into PostgreSQL. In master, I
would like to fully resynchronize with fresh imath 1.29. We're better off
naively tracking upstream than reactively maintaining a twelve-year-old
snapshot of upstream.
imath1.29-raw-sync-v1.patch is the result of copying new imath.c and imath.h
into place, removing "#ifdef __cplusplus" blocks that upset pgindent, running
pgindent, and filtering through "unexpand -t4 --first-only".
imath1.29-pgedits-v1.patch then restores PostgreSQL-specific changes. I would
squash these together for eventual commit, since the intermediate state is
broken, but it should ease review to see them separately. I did not keep the
INVERT_COMPARE_RESULT() change from c87cb5f; the domain of the comparisons in
question is {-1,0,1}, controlled entirely by code in imath.c.
Upstream has fixed bugs over the years, but I am not specifically aware of any
represented fix here that affects pgcrypto. Most suspicious to me are the
division fixes, which could affect our pgp_elgamal_{en,de}crypt(). You can
examine https://github.com/creachadair/imath/blob/master/ChangeLog for all
changes between imath-1.3 and imath-1.29.
Like PostgreSQL, imath now assumes C99. Unlike PostgreSQL, it has adopted
mixed declarations and code; our -Wdeclaration-after-statement would add
sixty-two warnings. If the compiler supports -Wdeclaration-after-statement, I
add -Wno-declaration-after-statement for imath.c.
Thanks,
nm
Attachment
>>>>> "Noah" == Noah Misch <noah@leadboat.com> writes: Noah> If the compiler supports -Wdeclaration-after-statement, I add Noah> -Wno-declaration-after-statement for imath.c. I found it much simpler to strip out -Wdeclaration-after-statement instead: $(RYU_OBJS): override CFLAGS := $(filter-out -Wdeclaration-after-statement,$(CFLAGS)) -- Andrew (irc:RhodiumToad)
On Sun, Feb 03, 2019 at 06:01:51AM +0000, Andrew Gierth wrote:
> >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:
>
> Noah> If the compiler supports -Wdeclaration-after-statement, I add
> Noah> -Wno-declaration-after-statement for imath.c.
>
> I found it much simpler to strip out -Wdeclaration-after-statement
> instead:
>
> $(RYU_OBJS): override CFLAGS := $(filter-out -Wdeclaration-after-statement,$(CFLAGS))
The -Wno-declaration-after-statement approach takes eight lines of code, and
the filter-out approach takes one. On the other hand, using $(filter-out)
changes any runs of whitespace to single spaces ("$(filter-out foo,a b c)"
yields "a b c"). We do risk that with CPPFLAGS and LDFLAGS in a few places.
I don't want to proliferate that practice, because it changes semantics of
CFLAGS containing -DFOO="arbitrary text".
>>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:
>> I found it much simpler to strip out -Wdeclaration-after-statement
>> instead:
>>
>> $(RYU_OBJS): override CFLAGS := $(filter-out -Wdeclaration-after-statement,$(CFLAGS))
Noah> The -Wno-declaration-after-statement approach takes eight lines
Noah> of code, and the filter-out approach takes one. On the other
Noah> hand, using $(filter-out) changes any runs of whitespace to
Noah> single spaces ("$(filter-out foo,a b c)" yields "a b c"). We do
Noah> risk that with CPPFLAGS and LDFLAGS in a few places. I don't want
Noah> to proliferate that practice, because it changes semantics of
Noah> CFLAGS containing -DFOO="arbitrary text".
In that case I propose that we avoid the whole issue by removing
-Wdeclaration-after-statement entirely.
So far, expressed opinions have run about 4:2 in favour of allowing
mixed declarations and code.
--
Andrew (irc:RhodiumToad)
>>>>> "Noah" == Noah Misch <noah@leadboat.com> writes: Noah> - # -Wdeclaration-after-statement isn't applicable for C++ Noah> + # -Wdeclaration-after-statement isn't applicable for C++. Specific C files Noah> + # disable it, so AC_SUBST the negative form. Noah> + PERMIT_DECLARATION_AFTER_STATEMENT= Noah> + if test x"$save_CFLAGS" != "$CFLAGS"; then Missing "x" here? + if test x"$save_CFLAGS" != x"$CFLAGS"; then -- Andrew (irc:RhodiumToad)
On Sun, Feb 03, 2019 at 07:07:36AM +0000, Andrew Gierth wrote: > In that case I propose that we avoid the whole issue by removing > -Wdeclaration-after-statement entirely. Some folks who skip $SUBJECT will be interested in your proposal. If you wish to pursue that proposal, please start a new thread. On Sun, Feb 03, 2019 at 07:14:13AM +0000, Andrew Gierth wrote: > >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes: > > Noah> - # -Wdeclaration-after-statement isn't applicable for C++ > Noah> + # -Wdeclaration-after-statement isn't applicable for C++. Specific C files > Noah> + # disable it, so AC_SUBST the negative form. > Noah> + PERMIT_DECLARATION_AFTER_STATEMENT= > Noah> + if test x"$save_CFLAGS" != "$CFLAGS"; then > > Missing "x" here? > > + if test x"$save_CFLAGS" != x"$CFLAGS"; then I'll incorporate your fix. Thanks.
Noah Misch <noah@leadboat.com> writes:
> The -Wno-declaration-after-statement approach takes eight lines of code, and
> the filter-out approach takes one. On the other hand, using $(filter-out)
> changes any runs of whitespace to single spaces ("$(filter-out foo,a b c)"
> yields "a b c"). We do risk that with CPPFLAGS and LDFLAGS in a few places.
> I don't want to proliferate that practice, because it changes semantics of
> CFLAGS containing -DFOO="arbitrary text".
I don't particularly buy that argument, because CPPFLAGS is where any -D
switches ought to be put. So we've already exposed ourselves to this
risk, in the unlikely scenario where it's not hypothetical.
regards, tom lane
On Sun, Feb 03, 2019 at 07:07:36AM +0000, Andrew Gierth wrote:
> >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:
>
> >> I found it much simpler to strip out -Wdeclaration-after-statement
> >> instead:
> >>
> >> $(RYU_OBJS): override CFLAGS := $(filter-out -Wdeclaration-after-statement,$(CFLAGS))
>
> Noah> The -Wno-declaration-after-statement approach takes eight lines
> Noah> of code, and the filter-out approach takes one. On the other
> Noah> hand, using $(filter-out) changes any runs of whitespace to
> Noah> single spaces ("$(filter-out foo,a b c)" yields "a b c"). We do
> Noah> risk that with CPPFLAGS and LDFLAGS in a few places. I don't want
> Noah> to proliferate that practice, because it changes semantics of
> Noah> CFLAGS containing -DFOO="arbitrary text".
>
> In that case I propose that we avoid the whole issue by removing
> -Wdeclaration-after-statement entirely.
>
> So far, expressed opinions have run about 4:2 in favour of allowing
> mixed declarations and code.
+1 for this.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On Sun, Feb 03, 2019 at 10:31:26AM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > The -Wno-declaration-after-statement approach takes eight lines of code, and
> > the filter-out approach takes one. On the other hand, using $(filter-out)
> > changes any runs of whitespace to single spaces ("$(filter-out foo,a b c)"
> > yields "a b c"). We do risk that with CPPFLAGS and LDFLAGS in a few places.
> > I don't want to proliferate that practice, because it changes semantics of
> > CFLAGS containing -DFOO="arbitrary text".
>
> I don't particularly buy that argument, because CPPFLAGS is where any -D
> switches ought to be put. So we've already exposed ourselves to this
> risk, in the unlikely scenario where it's not hypothetical.
The $(filter-out) corruption is unlikely to matter, indeed. The question is
whether to use eight lines of code to inject -Wno-declaration-after-statement
or one line to remove -Wdeclaration-after-statement using $(filter-out). I
see negligible drawbacks on either side; both approaches are tolerable. The
above-described hypothetical problem tips the scale in favor of
-Wno-declaration-after-statement.
On 2019-Feb-03, David Fetter wrote: > On Sun, Feb 03, 2019 at 07:07:36AM +0000, Andrew Gierth wrote: > > >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes: > > So far, expressed opinions have run about 4:2 in favour of allowing > > mixed declarations and code. > > +1 for this. Just curious, why do you care? If you want to use such constructs in extension code, you can add it in your makefiles. I'm -1 for this myself. I think there are a few places that could benefit from it, but my fear is that many *more* places would get worse. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On February 6, 2019 5:17:50 AM GMT+05:30, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >On 2019-Feb-03, David Fetter wrote: > >> On Sun, Feb 03, 2019 at 07:07:36AM +0000, Andrew Gierth wrote: >> > >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes: > >> > So far, expressed opinions have run about 4:2 in favour of allowing >> > mixed declarations and code. >> >> +1 for this. > >Just curious, why do you care? If you want to use such constructs in >extension code, you can add it in your makefiles. > >I'm -1 for this myself. I think there are a few places that could >benefit from it, but my fear is that many *more* places would get >worse. Because of imported code like ryu and imath? And because it can make code considerably better when used judiciously. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Andres Freund <andres@anarazel.de> writes:
> On February 6, 2019 5:17:50 AM GMT+05:30, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> I'm -1 for this myself. I think there are a few places that could
>> benefit from it, but my fear is that many *more* places would get
>> worse.
> Because of imported code like ryu and imath? And because it can make code considerably better when used judiciously.
I don't object to keeping imported code in a form that matches upstream
as best we can. (Should we also exclude such files from pgindent'ing?)
But changing conventions for our own code is an entirely different matter.
In this case, I think that having some places use it while the bulk of
the code doesn't is just a bad idea from a stylistic-consistency
standpoint. It's pretty much the same reason why we still aren't allowing
// comments --- there's no toolchain-based reason not to, but a mishmash of
comment styles would be ugly and hard to read.
regards, tom lane
Hi, On 2019-02-06 10:15:24 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On February 6, 2019 5:17:50 AM GMT+05:30, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > >> I'm -1 for this myself. I think there are a few places that could > >> benefit from it, but my fear is that many *more* places would get > >> worse. > > > Because of imported code like ryu and imath? And because it can make code considerably better when used judiciously. > > I don't object to keeping imported code in a form that matches upstream > as best we can. (Should we also exclude such files from pgindent'ing?) I think pgindenting doesn't matter as much, as that's an automated change. But as pgindent doesn't fix the position of declarations, that's a significant manual process. > But changing conventions for our own code is an entirely different matter. > In this case, I think that having some places use it while the bulk of > the code doesn't is just a bad idea from a stylistic-consistency > standpoint. It's pretty much the same reason why we still aren't allowing > // comments --- there's no toolchain-based reason not to, but a mishmash of > comment styles would be ugly and hard to read. Consistency surely has value, but it shouldn't block making use of new things that become available. I don't feel extremely strongly about allowing declarations after statements in C (while it's obviously crucial for C++), but I do think it can be a noticably easier to reason about the values of variables if they're declared closer to use, and it's easier to make sure that a variable hasn't been used elsewhere that way. Greetings, Andres Freund
On Wed, Feb 06, 2019 at 10:15:24AM -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On February 6, 2019 5:17:50 AM GMT+05:30, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > >> I'm -1 for this myself. I think there are a few places that could > >> benefit from it, but my fear is that many *more* places would get > >> worse. > > > Because of imported code like ryu and imath? And because it can make code considerably better when used judiciously. > > I don't object to keeping imported code in a form that matches upstream > as best we can. (Should we also exclude such files from pgindent'ing?) I think it depends on how much time one spends merging upstream changes versus making PostgreSQL-specific edits. For IMath, both amounts are too small to get excited about. Does pgindent materially complicate src/timezone merges? > But changing conventions for our own code is an entirely different matter. > In this case, I think that having some places use it while the bulk of > the code doesn't is just a bad idea from a stylistic-consistency > standpoint. It's pretty much the same reason why we still aren't allowing > // comments --- there's no toolchain-based reason not to, but a mishmash of > comment styles would be ugly and hard to read. (This debate never belonged in this thread, but it's too late now.) I find code easiest to follow when the declaration appears as late as possible. I would welcome mixed declarations and code, and I would not mourn the loss of consistency. In terms of consistency damage, this is similar to adding psprintf() without exterminating palloc()+snprintf(). I'm glad we introduce ways to write new, cleaner code despite the inconsistency with older code.
Noah Misch <noah@leadboat.com> writes:
> On Wed, Feb 06, 2019 at 10:15:24AM -0500, Tom Lane wrote:
>> I don't object to keeping imported code in a form that matches upstream
>> as best we can. (Should we also exclude such files from pgindent'ing?)
> I think it depends on how much time one spends merging upstream changes versus
> making PostgreSQL-specific edits. For IMath, both amounts are too small to
> get excited about. Does pgindent materially complicate src/timezone merges?
My practice with src/timezone is to pgindent the upstream code and then
diff it; given that extra step, it's not really any more complex (and
maybe less so, as this hides minor whitespace changes for instance).
There are some other deltas to worry about as well, see
src/timezone/README.
I have no particular opinion on whether pgindent should be part of the
mix for imath, but I do strongly recommend setting up and documenting a
reproducible import process, as I did for src/timezone.
regards, tom lane
> On 7 Feb 2019, at 16:12, Tom Lane <tgl@sss.pgh.pa.us> wrote: > .. I do strongly recommend setting up and documenting a > reproducible import process, as I did for src/timezone. Strong +1 on this. cheers ./daniel
On Thu, Feb 07, 2019 at 09:23:43PM +0100, Daniel Gustafsson wrote: > On 7 Feb 2019, at 16:12, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> .. I do strongly recommend setting up and documenting a >> reproducible import process, as I did for src/timezone. > > Strong +1 on this. +1. -- Michael
Attachment
On Thu, Feb 07, 2019 at 10:12:05AM -0500, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Wed, Feb 06, 2019 at 10:15:24AM -0500, Tom Lane wrote: > >> I don't object to keeping imported code in a form that matches upstream > >> as best we can. (Should we also exclude such files from pgindent'ing?) > > > I think it depends on how much time one spends merging upstream changes versus > > making PostgreSQL-specific edits. For IMath, both amounts are too small to > > get excited about. Does pgindent materially complicate src/timezone merges? > > My practice with src/timezone is to pgindent the upstream code and then > diff it; given that extra step, it's not really any more complex (and > maybe less so, as this hides minor whitespace changes for instance). Let's continue to pgindent src/timezone and IMath, then. > There are some other deltas to worry about as well, see > src/timezone/README. > > I have no particular opinion on whether pgindent should be part of the > mix for imath, but I do strongly recommend setting up and documenting a > reproducible import process, as I did for src/timezone. Good idea. I've modified the imath.c header comment to take the form of an import process; see attached imath1.29-pgedits-v2.patch.
Attachment
Noah Misch <noah@leadboat.com> writes:
> On Thu, Feb 07, 2019 at 10:12:05AM -0500, Tom Lane wrote:
>> I have no particular opinion on whether pgindent should be part of the
>> mix for imath, but I do strongly recommend setting up and documenting a
>> reproducible import process, as I did for src/timezone.
> Good idea. I've modified the imath.c header comment to take the form of an
> import process; see attached imath1.29-pgedits-v2.patch.
Maybe easier to keep the instructions in a separate README file?
(I don't see a need to put a PG copyright in this file, when the
mods from upstream are this small.)
Looks good otherwise.
regards, tom lane
On Thu, Feb 07, 2019 at 11:56:18PM -0500, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Thu, Feb 07, 2019 at 10:12:05AM -0500, Tom Lane wrote: > >> I have no particular opinion on whether pgindent should be part of the > >> mix for imath, but I do strongly recommend setting up and documenting a > >> reproducible import process, as I did for src/timezone. > > > Good idea. I've modified the imath.c header comment to take the form of an > > import process; see attached imath1.29-pgedits-v2.patch. > > Maybe easier to keep the instructions in a separate README file? Most imath.c patches have cause to update those lines, and patches to other files almost never do. Hence, I think hackers are more likely to find and update those lines in imath.c. I would choose a README if concerns weren't concentrated in one file. > (I don't see a need to put a PG copyright in this file, when the > mods from upstream are this small.) The tree has been inconsistent about that, which is okay. I was mimicking src/port/strlcpy.c. Others, e.g. src/port/crypt.c, haven't added a copyright.
Noah Misch <noah@leadboat.com> writes:
> On Thu, Feb 07, 2019 at 11:56:18PM -0500, Tom Lane wrote:
>> Maybe easier to keep the instructions in a separate README file?
> Most imath.c patches have cause to update those lines, and patches to other
> files almost never do. Hence, I think hackers are more likely to find and
> update those lines in imath.c. I would choose a README if concerns weren't
> concentrated in one file.
Fair enough, objection withdrawn.
regards, tom lane
On Sat, Feb 16, 2019 at 02:12:50PM -0500, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Thu, Feb 07, 2019 at 11:56:18PM -0500, Tom Lane wrote: > >> Maybe easier to keep the instructions in a separate README file? > > > Most imath.c patches have cause to update those lines, and patches to other > > files almost never do. Hence, I think hackers are more likely to find and > > update those lines in imath.c. I would choose a README if concerns weren't > > concentrated in one file. > > Fair enough, objection withdrawn. Pushed, but it broke dory. Will fix.