Обсуждение: pgsql: Fix for globals.c- c.h must come first
Fix for globals.c- c.h must come first Commit da9b580 mistakenly put a system header before postgres.h (which includes c.h). That can cause portability issues and broke (at least) builds with older Windows compilers. Discovered by Mark Dilger. Discussion: https://postgr.es/m/BF04A27A-D132-4927-A80A-BAD18695E954@gmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/e2b83ff556deb9a0001bdf6b511f8cfc9189ac10 Modified Files -------------- src/backend/utils/init/globals.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
On Sat, May 19, 2018 at 01:20:47AM +0000, Stephen Frost wrote: > Fix for globals.c- c.h must come first > > Commit da9b580 mistakenly put a system header before postgres.h (which > includes c.h). That can cause portability issues and broke (at least) > builds with older Windows compilers. I assume there is no way to add defined and checks to globals.c and c.h to cause a compile error when this happens. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
On 2018-06-19 13:58:34 -0400, Bruce Momjian wrote: > On Sat, May 19, 2018 at 01:20:47AM +0000, Stephen Frost wrote: > > Fix for globals.c- c.h must come first > > > > Commit da9b580 mistakenly put a system header before postgres.h (which > > includes c.h). That can cause portability issues and broke (at least) > > builds with older Windows compilers. > > I assume there is no way to add defined and checks to globals.c and c.h > to cause a compile error when this happens. I don't see how to do so in a form that's even halfway portable. Just to be clear: There's nothing globals.c specific about the rule to always include postgres.h first. Greetings, Andres Freund
Bruce Momjian <bruce@momjian.us> writes:
> On Sat, May 19, 2018 at 01:20:47AM +0000, Stephen Frost wrote:
>> Commit da9b580 mistakenly put a system header before postgres.h (which
>> includes c.h). That can cause portability issues and broke (at least)
>> builds with older Windows compilers.
> I assume there is no way to add defined and checks to globals.c and c.h
> to cause a compile error when this happens.
Hm. You could imagine adding something like
#ifdef some-relevant-macro
#error include ordering problem, c.h must come before system headers
#endif
near the head of c.h. But I'm not sure how we'd get full coverage.
The case presumably would only occur for off-the-beaten-path headers
(this particular mistake was <sys/stat.h>), and there are lots of those.
regards, tom lane
On 2018-06-19 14:09:15 -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Sat, May 19, 2018 at 01:20:47AM +0000, Stephen Frost wrote: > >> Commit da9b580 mistakenly put a system header before postgres.h (which > >> includes c.h). That can cause portability issues and broke (at least) > >> builds with older Windows compilers. > > > I assume there is no way to add defined and checks to globals.c and c.h > > to cause a compile error when this happens. > > Hm. You could imagine adding something like > > #ifdef some-relevant-macro > #error include ordering problem, c.h must come before system headers > #endif > > near the head of c.h. But I'm not sure how we'd get full coverage. > The case presumably would only occur for off-the-beaten-path headers > (this particular mistake was <sys/stat.h>), and there are lots of those. If we were ok with doing this in a very platform specific manner, we probably could make the dependency fairly generic. I.e. glibc IIRC will pretty much always end up including features.h, so we could check for that (#ifdef _FEATURES_H). I'd suspect other C libraries have similar things. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2018-06-19 14:09:15 -0400, Tom Lane wrote:
>> Hm. You could imagine adding something like
>>
>> #ifdef some-relevant-macro
>> #error include ordering problem, c.h must come before system headers
>> #endif
>>
>> near the head of c.h. But I'm not sure how we'd get full coverage.
>> The case presumably would only occur for off-the-beaten-path headers
>> (this particular mistake was <sys/stat.h>), and there are lots of those.
> If we were ok with doing this in a very platform specific manner, we
> probably could make the dependency fairly generic. I.e. glibc IIRC will
> pretty much always end up including features.h, so we could check for
> that (#ifdef _FEATURES_H). I'd suspect other C libraries have similar
> things.
It'd probably be good enough if the error detection worked with glibc's
headers. Even if nobody noticed before a particular patch went in,
the buildfarm would surely find it.
regards, tom lane
I wrote:
> It'd probably be good enough if the error detection worked with glibc's
> headers. Even if nobody noticed before a particular patch went in,
> the buildfarm would surely find it.
On second thought, we'd also need a solution for Windows, because we
have some files that only get compiled for Windows. But even two
separate tests of this sort seems reasonable.
regards, tom lane
Hi, On 2018-06-19 14:19:56 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2018-06-19 14:09:15 -0400, Tom Lane wrote: > >> Hm. You could imagine adding something like > >> > >> #ifdef some-relevant-macro > >> #error include ordering problem, c.h must come before system headers > >> #endif > >> > >> near the head of c.h. But I'm not sure how we'd get full coverage. > >> The case presumably would only occur for off-the-beaten-path headers > >> (this particular mistake was <sys/stat.h>), and there are lots of those. > > > If we were ok with doing this in a very platform specific manner, we > > probably could make the dependency fairly generic. I.e. glibc IIRC will > > pretty much always end up including features.h, so we could check for > > that (#ifdef _FEATURES_H). I'd suspect other C libraries have similar > > things. > > It'd probably be good enough if the error detection worked with glibc's > headers. Even if nobody noticed before a particular patch went in, > the buildfarm would surely find it. Then the _FEATURES_H check would work - just tested it locally, and it triggers appropriately. Does tie us a bit to specific glibc versions, but... Obvoiusly would need some comments explaining it Are there arguments for doing this in postgres.h rather than c.h? I can't immediately think of a case where it'd be ok for postgres_fe.h or such to be included later. libpq-fe would be different, but that doesn't include backend headers. I think we shouldn't do this for v11, as it seems likely that some extensions would be broken. While not hard to fix, that seems unnecessary after beta1? Seems unlikely we'll introduce further cases before new features are merged. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Are there arguments for doing this in postgres.h rather than c.h?
No, c.h is the correct place. The core problem is that we have to include
pg_config.h before <stdio.h> et al in order to have consistent libc APIs
(on platforms where this matters), and c.h is what does that.
> I think we shouldn't do this for v11, as it seems likely that some
> extensions would be broken. While not hard to fix, that seems
> unnecessary after beta1? Seems unlikely we'll introduce further cases
> before new features are merged.
Yeah, waiting for v12 seems fine.
regards, tom lane
Hi, On 2018-06-19 14:22:25 -0400, Tom Lane wrote: > I wrote: > > It'd probably be good enough if the error detection worked with glibc's > > headers. Even if nobody noticed before a particular patch went in, > > the buildfarm would surely find it. > > On second thought, we'd also need a solution for Windows, because we > have some files that only get compiled for Windows. But even two > separate tests of this sort seems reasonable. Could anybody working on windows (Amit, Michael?) check whether there's an equivalent to my _FEATURES_H trick? Greetings, Andres Freund