Обсуждение: Unportable coding in reorderbuffer.h
I don't believe that this is legal per C90:
typedef struct ReorderBufferChange
{ XLogRecPtr lsn;
/* type of change */ union { enum ReorderBufferChangeType action; /* do not leak internal enum
valuesto the outside */ int action_internal; };
...
That union field needs a name. Yeah, I realize this makes references
to the contained fields more verbose. Tough. Our project standard
is we compile on C90 compilers, and we're not moving that goalpost
just to save you a couple of keystrokes.
Worse, this isn't portable even if you assume a C99 compiler. There is
nothing at all constraining the compiler to make the enum field the same
size as the int field, and if they're not, reading the int will not yield
the same value you wrote as an enum (or vice versa). It might've
accidentally failed to fail on the compilers you tested on, but it's
still wrong.
The other nameless union in ReorderBufferChange needs a name too.
By the time you get done fixing the portability issue, I suspect you
won't have a union at all for the first case. I'm not real sure that
you need a union for the second case either --- is it really important
to shave a few bytes from the size of this struct? So you don't
necessarily need to do a notation change for the second union.
What drew my attention to it was an older gcc version complaining
thusly:
In file included from ../../../../src/include/replication/decode.h:13, from decode.c:38:
../../../../src/include/replication/reorderbuffer.h:60: warning: unnamed struct/union that defines no instances
../../../../src/include/replication/reorderbuffer.h:94: warning: unnamed struct/union that defines no instances
decode.c: In function `DecodeInsert':
decode.c:588: structure has no member named `action'
decode.c:589: structure has no member named `tp'
decode.c:595: structure has no member named `tp'
decode.c:599: structure has no member named `tp'
decode.c: In function `DecodeUpdate':
decode.c:628: structure has no member named `action'
decode.c:629: structure has no member named `tp'
decode.c:637: structure has no member named `tp'
decode.c:641: structure has no member named `tp'
decode.c:651: structure has no member named `tp'
decode.c:654: structure has no member named `tp'
... etc etc ...
regards, tom lane
On 2014-03-05 17:40:56 -0500, Tom Lane wrote:
> I don't believe that this is legal per C90:
>
> typedef struct ReorderBufferChange
> {
> XLogRecPtr lsn;
>
> /* type of change */
> union
> {
> enum ReorderBufferChangeType action;
> /* do not leak internal enum values to the outside */
> int action_internal;
> };
>
> ...
>
> That union field needs a name.
You're absolutely right.
> Our project standard is we compile on C90 compilers, and we're not
> moving that goalpost just to save you a couple of keystrokes.
While it's a good idea to try to save keystrokes the actual reason is
that I just had somehow forgotten that it hasn't always been
supported. I think it's not even C99, but C11...
> By the time you get done fixing the portability issue, I suspect you
> won't have a union at all for the first case.
You might be right. I'd rather not leak the internal enum values to the
public though, so there are two choices:
* define as enum, but store values in the that aren't actually valid members. IIRC that's legal, even if not pretty.
Anythingoutside reorderbuffer.c doesn't ever see the values that aren't enum values.
* define it as an int, but suggest casting to the enum inside switch() in examples/docs.
> I'm not real sure that
> you need a union for the second case either --- is it really important
> to shave a few bytes from the size of this struct? So you don't
> necessarily need to do a notation change for the second union.
I think it's allocated frequently enough to warrant that
unfortunately. Changing that will be fun.
> What drew my attention to it was an older gcc version complaining
> thusly: [errors]
And there I was, suprised it hadn't turned the buildfarm red.
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-03-05 17:40:56 -0500, Tom Lane wrote:
>> By the time you get done fixing the portability issue, I suspect you
>> won't have a union at all for the first case.
> You might be right. I'd rather not leak the internal enum values to the
> public though, so there are two choices:
> * define as enum, but store values in the that aren't actually valid
> members. IIRC that's legal, even if not pretty. Anything outside
> reorderbuffer.c doesn't ever see the values that aren't enum values.
> * define it as an int, but suggest casting to the enum inside switch()
> in examples/docs.
Meh. I think you're being *way* too cute here. I'd just put all the
values in one enum declaration, and use comments and/or naming convention
to mark some of them as internal and not meant to be used outside the
reorderbuffer stuff. That will for one thing allow gdb to print all
the values properly. And somebody who's bound and determined to violate
the abstraction could do it anyway, no?
>> What drew my attention to it was an older gcc version complaining
>> thusly: [errors]
> And there I was, suprised it hadn't turned the buildfarm red.
I'm surprised too; I had thought we still had some critters running
hoary compilers. We need to do something about that if we actually
believe in C90-compiler support.
I experimented a little bit and found that modern gcc with -ansi
(aka -std=c89) will complain about this particular issue, and it also
complains about // comments which are a perpetual hazard; but it is still
happy about a lot of other not-C90 things like flexible array members.
We could try spinning up a buildfarm critter with -ansi -pedantic
but I'm not sure if that's much better. The GCC man page warns that
these options are not meant to be a strict check for C90 standard
compliance, and anyway the important question is not whether gcc with
options that nobody uses will take our code, its whether old compilers
that are still in real use will take it.
For a long time I was using gcc 2.95.3 on my HPPA dinosaur as a
reference for hoary-compiler behavior. That machine is now flaky
enough that I don't want to keep it turned on 24/7, but maybe I could
compile up 2.95.3 on some other box and start a buildfarm critter.
regards, tom lane
On 2014-03-05 19:12:15 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > On 2014-03-05 17:40:56 -0500, Tom Lane wrote: > >> By the time you get done fixing the portability issue, I suspect you > >> won't have a union at all for the first case. > > > You might be right. I'd rather not leak the internal enum values to the > > public though, so there are two choices: > > * define as enum, but store values in the that aren't actually valid > > members. IIRC that's legal, even if not pretty. Anything outside > > reorderbuffer.c doesn't ever see the values that aren't enum values. > > * define it as an int, but suggest casting to the enum inside switch() > > in examples/docs. > > Meh. I think you're being *way* too cute here. I'd just put all the > values in one enum declaration, and use comments and/or naming convention > to mark some of them as internal and not meant to be used outside the > reorderbuffer stuff. That will for one thing allow gdb to print all > the values properly. And somebody who's bound and determined to violate > the abstraction could do it anyway, no? The reason I'd like to avoid the internal members in the public enum isn't so much that I want to avoid the internal names being visible to the outside, but that I find it very helpful to see compile time warnings about public enum values not being handled in output plugins. I am pretty sure we're going to add further features in the not too far away futures and it seems nicer to be able to warn that way. But I guess it's too much work, for not enough benefit :( > >> What drew my attention to it was an older gcc version complaining > >> thusly: [errors] > > > And there I was, suprised it hadn't turned the buildfarm red. > > I'm surprised too; I had thought we still had some critters running > hoary compilers. We need to do something about that if we actually > believe in C90-compiler support. What version was the gcc that triggered the error? I have to admit that I am really not interested in supporting gcc 2.95 , that thing is just too old (nearly 15 years!) and buggy. But it's not a small step from desupporting 2.95 to having gcc 3.4 (protosciurus, narwahl) as the minimum. And it should be a conscious not implicit decision. It'd be really helpful to have some more buildfarm animals running real-world relevant older compilers. I'd be surprised if this is the only such issue lurking around. As I've compiled it anyway while thinking about gcc versions, here's the lifetimes of various gcc releases: version first version last version --- 2.95 July 31, 1999 March 16, 2001 3.0 June 18, 2001 December 20, 2001 3.1 May 15, 2002 July 27, 2002 3.2 August 14, 2002 April 25, 2003 3.3 May 14, 2003 May 3, 2005 3.4 April 18, 2004 March 6, 2006 4.0 April 20, 2005 January 31, 2007 4.1 February 28, 2006 February 13, 2007 4.2 May 13, 2007 May 19, 2008 4.3 March 5, 2008 Jun 27, 2011 4.4 April 21, 2009 March 13, 2012 4.5 April 14, 2010 Jul 2, 2012 4.6 March 25, 2011 April 12, 2013 4.7 March 22, 2012 - 4.8 March 22, 2013 - --- I personally think it's time to dump some older compiler versions, and adopt at least individual C99 features (e.g. static inlines). Greetings, Andres Freund
On Wed, Mar 5, 2014 at 6:50 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-03-05 17:40:56 -0500, Tom Lane wrote:
>> I don't believe that this is legal per C90:
>>
>> typedef struct ReorderBufferChange
>> {
>> XLogRecPtr lsn;
>>
>> /* type of change */
>> union
>> {
>> enum ReorderBufferChangeType action;
>> /* do not leak internal enum values to the outside */
>> int action_internal;
>> };
>>
>> ...
>>
>> That union field needs a name.
>
> You're absolutely right.
>
>> Our project standard is we compile on C90 compilers, and we're not
>> moving that goalpost just to save you a couple of keystrokes.
>
> While it's a good idea to try to save keystrokes the actual reason is
> that I just had somehow forgotten that it hasn't always been
> supported. I think it's not even C99, but C11...
Urgh. I know that isn't per project style, but I just plain missed it
while staring at these patches. At one point I thought of complaining
that separating the public and private values was not a worthwhile
endeavor, but I don't think I ever did. Still, I agree with Tom's
suggestion of dumping the distinction.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-03-05 19:12:15 -0500, Tom Lane wrote:
>> I'm surprised too; I had thought we still had some critters running
>> hoary compilers. We need to do something about that if we actually
>> believe in C90-compiler support.
> What version was the gcc that triggered the error?
That was the 2.95.3 I have on my HPPA box. I don't have any 3.x versions
in captivity; the next oldest I have is 4.0.1 on a Mac (running Leopard
or thereabouts), and it seems to take this code.
> I have to admit that I am really not interested in supporting gcc 2.95 ,
> that thing is just too old (nearly 15 years!) and buggy.
[ shrug... ] In 15 years, the only problem I've seen with 2.95.3 is
that it's prone to complaining about variables-clobbered-by-longjmp
that no other compiler is unhappy with. Maybe the HPPA build is less
buggy due to less aggressive optimization? I usually use -O1 with it,
and that backend might be less tense than the Intel backend anyway.
However, this is probably a bit beside the point. I'm quite prepared
to believe that nobody uses gcc < 4.0 anymore. The question is what
older non-gcc compilers are still out there, and can we either get hold
of them for the buildfarm, or trust that a really old gcc will complain
about the same things they would? I suspect that most of the candidates
would be proprietary compilers, so short of shelling out license fees
I think we might be stuck with using old gcc as a proxy. As I said,
I've more often than not found that things 2.95.3 will take don't
cause problems elsewhere.
> I personally think it's time to dump some older compiler versions, and
> adopt at least individual C99 features (e.g. static inlines).
Meh. In the first place, what you want is not C99 inlines it's GNU
inlines; the standard's version is brain-dead. But I'm not prepared
to declare us a GCC-only shop. In the second place, we already have a
workable if slightly klugy solution for GNU inlines without assuming
all compilers do that. I don't see a need to throw that overboard.
regards, tom lane
On 2014-03-05 20:03:16 -0500, Tom Lane wrote: > However, this is probably a bit beside the point. I'm quite prepared > to believe that nobody uses gcc < 4.0 anymore. The question is what > older non-gcc compilers are still out there, and can we either get hold > of them for the buildfarm, or trust that a really old gcc will complain > about the same things they would? I suspect that most of the candidates > would be proprietary compilers, so short of shelling out license fees > I think we might be stuck with using old gcc as a proxy. I wonder if it makes sense to send out an -announce email asking for critters if they want $platform to stay supported. No response doesn't imply explicitly desupporting $platform... The channels where I recall request for critters on old platforms being made don't have a very high circulation (e.g. -bugs). > > I personally think it's time to dump some older compiler versions, and > > adopt at least individual C99 features (e.g. static inlines). > > Meh. In the first place, what you want is not C99 inlines it's GNU > inlines; the standard's version is brain-dead. That's true for several inline variants (extern inline, inline without extern), but unless I am severely misremembering not for static inlines. And static inline is what I am interested in. > But I'm not prepared to declare us a GCC-only shop. All platforms, even the hpux critter that existed till some time ago, do support static inline. It's only that the hpux critter warned about unused functions, but even that could have been disabled with a compiler flag. > In the second place, we already have a > workable if slightly klugy solution for GNU inlines without assuming > all compilers do that. Meh squared. As the person first starting with the STATIC_IF_INLINE thing I am readily declaring it a utterly horrible crock. And not one suited for all things. I very much want to replace some of the uglier macros with inline functions. Some are very, very hard to understand and give utterly horrible error messages that are very hard to understand, even for experienced people. And several of those cannot be replaced by a extern function without indirectly making the respective non-static-inline-supporting platforms indirectly unsupported. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2014-03-05 20:02:56 -0500, Robert Haas wrote: > On Wed, Mar 5, 2014 at 6:50 PM, Andres Freund <andres@2ndquadrant.com> wrote: > Urgh. I know that isn't per project style, but I just plain missed it > while staring at these patches. At one point I thought of complaining > that separating the public and private values was not a worthwhile > endeavor, but I don't think I ever did. Still, I agree with Tom's > suggestion of dumping the distinction. Ok, convinced, consider it dumped. Unless somebody protests I'll try to get the remaining walsender and docs patches ready before sending in the patch fixing this as it's not disturbing the buildfarm. I'll be onsite/travelling tomorrow; so I am not sure I'll be done then, but friday it is. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Hi, On 2014-03-06 02:39:37 +0100, Andres Freund wrote: > Unless somebody protests I'll try to get the remaining walsender and > docs patches ready before sending in the patch fixing this as it's not > disturbing the buildfarm. I'll be onsite/travelling tomorrow; so I am > not sure I'll be done then, but friday it is. A patch fixing this is attached. I've added some more local variables to deal with the longer lines... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
Andres Freund <andres@2ndquadrant.com> writes:
> A patch fixing this is attached. I've added some more local variables to
> deal with the longer lines...
Since I've got a compiler in captivity that complains about this,
I'll take care of checking and committing this patch.
I still think it might be a good idea to spin up a buildfarm member
running "gcc -ansi -pedantic", even if we don't see that as a particularly
useful case in practice. Thoughts?
regards, tom lane
On 2014-03-07 13:27:28 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > A patch fixing this is attached. I've added some more local variables to > > deal with the longer lines... > > Since I've got a compiler in captivity that complains about this, > I'll take care of checking and committing this patch. Thanks. The tests specific for the feature can be run with (cd contrib/test_decoding && make -s check), as they require non-default PGC_POSTMASTER settings. > I still think it might be a good idea to spin up a buildfarm member > running "gcc -ansi -pedantic", even if we don't see that as a particularly > useful case in practice. Thoughts? I tried to compile with both -ansi -pedantic -std=c90 to catch potential further issues, but at least my gcc version makes the output completely drown in various warnings. Some can be individually disabled, but lots of them seem to be only be covered by pretty general warning categories. -ansi -std=c90 -Wno-variadic-macros works reasonably well tho. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services