Thread: windows cfbot failing: my_perl
The last 20 some consecutive builds failed: https://cirrus-ci.com/github/postgresql-cfbot/postgresql like this: [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl':undeclared identifier (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj] [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl':undeclared identifier (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj] [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of '->IMem'must point to struct/union (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj] [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of '->IMem'must point to struct/union (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj] I imagine it may be due to an error hit while rebuilding the ci's docker image. -- Justin
Hi, On 2022-08-26 06:55:46 -0500, Justin Pryzby wrote: > The last 20 some consecutive builds failed: > https://cirrus-ci.com/github/postgresql-cfbot/postgresql > > like this: > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl':undeclared identifier (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj] > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl':undeclared identifier (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj] > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of'->IMem' must point to struct/union (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj] > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of'->IMem' must point to struct/union (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj] > > I imagine it may be due to an error hit while rebuilding the ci's docker image. I don't think it's CI specific, see https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2022-08-26%2011%3A00%3A11 Looks like the failures might have started with https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=121d2d3d70ecdb2113b340c5f3b99a61341291af based on https://cirrus-ci.com/github/postgres/postgres/ Not immediately obvious why that would be. Greetings, Andres Freund
Hi, On 2022-08-26 06:21:51 -0700, Andres Freund wrote: > On 2022-08-26 06:55:46 -0500, Justin Pryzby wrote: > > The last 20 some consecutive builds failed: > > https://cirrus-ci.com/github/postgresql-cfbot/postgresql > > > > like this: > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl':undeclared identifier (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj] > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl':undeclared identifier (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj] > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of'->IMem' must point to struct/union (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj] > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of'->IMem' must point to struct/union (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj] > > > > I imagine it may be due to an error hit while rebuilding the ci's docker image. > > I don't think it's CI specific, see > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2022-08-26%2011%3A00%3A11 > > Looks like the failures might have started with > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=121d2d3d70ecdb2113b340c5f3b99a61341291af > based on > https://cirrus-ci.com/github/postgres/postgres/ > > Not immediately obvious why that would be. Reproduces in a VM, it starts to fail with that commit. Looks like somehow different macros are trampling on each other. Something in perl is interfering with msvc's malloc.h, turning if (_Marker == _ALLOCA_S_HEAP_MARKER) { free(_Memory); } into if (_Marker == 0xDDDD) { (*(my_perl->IMem)->pFree)((my_perl->IMem), (_Memory)); } after preprocessing. No idea how. Greetings, Andres Freund
Hi, On 2022-08-26 06:40:47 -0700, Andres Freund wrote: > On 2022-08-26 06:21:51 -0700, Andres Freund wrote: > > On 2022-08-26 06:55:46 -0500, Justin Pryzby wrote: > > > The last 20 some consecutive builds failed: > > > https://cirrus-ci.com/github/postgresql-cfbot/postgresql > > > > > > like this: > > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl':undeclared identifier (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj] > > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl':undeclared identifier (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj] > > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: leftof '->IMem' must point to struct/union (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj] > > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: leftof '->IMem' must point to struct/union (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj] > > > > > > I imagine it may be due to an error hit while rebuilding the ci's docker image. > > > > I don't think it's CI specific, see > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2022-08-26%2011%3A00%3A11 > > > > Looks like the failures might have started with > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=121d2d3d70ecdb2113b340c5f3b99a61341291af > > based on > > https://cirrus-ci.com/github/postgres/postgres/ > > > > Not immediately obvious why that would be. > > Reproduces in a VM, it starts to fail with that commit. Looks like somehow > different macros are trampling on each other. Something in perl is interfering > with msvc's malloc.h, turning > > if (_Marker == _ALLOCA_S_HEAP_MARKER) > { > free(_Memory); > } > into > > if (_Marker == 0xDDDD) > { > (*(my_perl->IMem)->pFree)((my_perl->IMem), (_Memory)); > } > > after preprocessing. No idea how. Because perl, extremely unhelpfully, #defines free. Which, not surprisingly, causes issues when including system headers referencing free as well. I don't really see a good solution to this other than hoisting the mb/pg_wchar.h include out to before we include all the perl stuff. That does fix the issue. Greetings, Andres Freund
On Fri, Aug 26, 2022 at 9:27 PM Andres Freund <andres@anarazel.de> wrote: > > Because perl, extremely unhelpfully, #defines free. Which, not surprisingly, > causes issues when including system headers referencing free as well. > > I don't really see a good solution to this other than hoisting the > mb/pg_wchar.h include out to before we include all the perl stuff. That does > fix the issue. We could also move is_valid_ascii somewhere else. It's only tangentially related to "wide chars" anyway. -- John Naylor EDB: http://www.enterprisedb.com
Hi, On 2022-08-26 21:39:05 +0700, John Naylor wrote: > On Fri, Aug 26, 2022 at 9:27 PM Andres Freund <andres@anarazel.de> wrote: > > > > Because perl, extremely unhelpfully, #defines free. Which, not surprisingly, > > causes issues when including system headers referencing free as well. > > > > I don't really see a good solution to this other than hoisting the > > mb/pg_wchar.h include out to before we include all the perl stuff. That does > > fix the issue. > > We could also move is_valid_ascii somewhere else. It's only > tangentially related to "wide chars" anyway. Given the crazy defines of stuff like free, it seems like a good idea to have a rule that no headers should be included after plperl.h with PG_NEED_PERL_XSUB_H defined. It's not like there's not other chances of of pulling in malloc.h from within pg_wchar.h somehow. It's a bit ugly to have the mb/pg_wchar.h in plperl.h instead of plperl_helpers.h, but ... Greetings, Andres Freund
Hi, Tom, Ilmari, you seem to have hacked on this stuff most (not so) recently. Do you have a better suggestion than moving the mb/pg_wchar.h include out of plperl_helpers.h as I suggest below? On 2022-08-26 07:47:40 -0700, Andres Freund wrote: > On 2022-08-26 21:39:05 +0700, John Naylor wrote: > > On Fri, Aug 26, 2022 at 9:27 PM Andres Freund <andres@anarazel.de> wrote: > > > > > > Because perl, extremely unhelpfully, #defines free. Which, not surprisingly, > > > causes issues when including system headers referencing free as well. > > > > > > I don't really see a good solution to this other than hoisting the > > > mb/pg_wchar.h include out to before we include all the perl stuff. That does > > > fix the issue. > > > > We could also move is_valid_ascii somewhere else. It's only > > tangentially related to "wide chars" anyway. > > Given the crazy defines of stuff like free, it seems like a good idea to have > a rule that no headers should be included after plperl.h with > PG_NEED_PERL_XSUB_H defined. It's not like there's not other chances of of > pulling in malloc.h from within pg_wchar.h somehow. > > It's a bit ugly to have the mb/pg_wchar.h in plperl.h instead of > plperl_helpers.h, but ... Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Tom, Ilmari, you seem to have hacked on this stuff most (not so) recently. Do
> you have a better suggestion than moving the mb/pg_wchar.h include out of
> plperl_helpers.h as I suggest below?
I agree with the conclusion that we'd better #include all our own
headers before any of Perl's. No strong opinions about which
rearrangement is least ugly --- but let's add some comments about
that requirement.
regards, tom lane
On 2022-08-26 Fr 10:47, Andres Freund wrote: > Hi, > > On 2022-08-26 21:39:05 +0700, John Naylor wrote: >> On Fri, Aug 26, 2022 at 9:27 PM Andres Freund <andres@anarazel.de> wrote: >>> Because perl, extremely unhelpfully, #defines free. Which, not surprisingly, >>> causes issues when including system headers referencing free as well. >>> >>> I don't really see a good solution to this other than hoisting the >>> mb/pg_wchar.h include out to before we include all the perl stuff. That does >>> fix the issue. >> We could also move is_valid_ascii somewhere else. It's only >> tangentially related to "wide chars" anyway. > Given the crazy defines of stuff like free, it seems like a good idea to have > a rule that no headers should be included after plperl.h with > PG_NEED_PERL_XSUB_H defined. It's not like there's not other chances of of > pulling in malloc.h from within pg_wchar.h somehow. > > It's a bit ugly to have the mb/pg_wchar.h in plperl.h instead of > plperl_helpers.h, but ... > It's already included directly in plperl.c, so couldn't we just lift it directly into SPI.xs and Util.xs? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Hi, On 2022-08-26 17:05:52 -0400, Andrew Dunstan wrote: > On 2022-08-26 Fr 10:47, Andres Freund wrote: > > Given the crazy defines of stuff like free, it seems like a good idea to have > > a rule that no headers should be included after plperl.h with > > PG_NEED_PERL_XSUB_H defined. It's not like there's not other chances of of > > pulling in malloc.h from within pg_wchar.h somehow. > > > > It's a bit ugly to have the mb/pg_wchar.h in plperl.h instead of > > plperl_helpers.h, but ... > It's already included directly in plperl.c, so couldn't we just lift it > directly into SPI.xs and Util.xs? I think it'd also be needed in hstore_plperl.c, jsonb_plperl.c. Putting the include in plperl.h would keep that aspect transparent, because plperl_utils.h includes plperl.h. I don't think manually including all dependencies, even if it's just one, in each of the six files currently using plperl_utils.h is a good approach. Greetings, Andres Freund
On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2022-08-26 17:05:52 -0400, Andrew Dunstan wrote: > > On 2022-08-26 Fr 10:47, Andres Freund wrote: > > > Given the crazy defines of stuff like free, it seems like a good idea to have > > > a rule that no headers should be included after plperl.h with > > > PG_NEED_PERL_XSUB_H defined. It's not like there's not other chances of of > > > pulling in malloc.h from within pg_wchar.h somehow. > > > > > > It's a bit ugly to have the mb/pg_wchar.h in plperl.h instead of > > > plperl_helpers.h, but ... > > > It's already included directly in plperl.c, so couldn't we just lift it > > directly into SPI.xs and Util.xs? > > I think it'd also be needed in hstore_plperl.c, jsonb_plperl.c. Putting the > include in plperl.h would keep that aspect transparent, because plperl_utils.h > includes plperl.h. Since plperl_helpers.h already includes plperl.h, I'm not sure why both are included everywhere the former is. If .c/.xs files didn't include plperl.h directly, we could keep pg_wchar.h in plperl_helpers.h. Not sure if that's workable or any better... -- John Naylor EDB: http://www.enterprisedb.com
John Naylor <john.naylor@enterprisedb.com> writes:
> On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres@anarazel.de> wrote:
>> I think it'd also be needed in hstore_plperl.c, jsonb_plperl.c. Putting the
>> include in plperl.h would keep that aspect transparent, because plperl_utils.h
>> includes plperl.h.
> Since plperl_helpers.h already includes plperl.h, I'm not sure why
> both are included everywhere the former is. If .c/.xs files didn't
> include plperl.h directly, we could keep pg_wchar.h in
> plperl_helpers.h. Not sure if that's workable or any better...
Maybe we should flush the separate plperl_helpers.h header and just
put those static-inline functions in plperl.h.
regards, tom lane
On Sat, Aug 27, 2022 at 10:02 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > John Naylor <john.naylor@enterprisedb.com> writes: > > On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres@anarazel.de> wrote: > >> I think it'd also be needed in hstore_plperl.c, jsonb_plperl.c. Putting the > >> include in plperl.h would keep that aspect transparent, because plperl_utils.h > >> includes plperl.h. > > > Since plperl_helpers.h already includes plperl.h, I'm not sure why > > both are included everywhere the former is. If .c/.xs files didn't > > include plperl.h directly, we could keep pg_wchar.h in > > plperl_helpers.h. Not sure if that's workable or any better... > > Maybe we should flush the separate plperl_helpers.h header and just > put those static-inline functions in plperl.h. Here's a patch with that idea, not tested on Windows yet. -- John Naylor EDB: http://www.enterprisedb.com
Attachment
On Sat, Aug 27, 2022 at 11:20 AM John Naylor <john.naylor@enterprisedb.com> wrote: > > Here's a patch with that idea, not tested on Windows yet. Update: I tried taking the CI for a spin, but ran into IT issues with Github when I tried to push my branch to remote. -- John Naylor EDB: http://www.enterprisedb.com
On 2022-08-26 23:02:06 -0400, Tom Lane wrote: > John Naylor <john.naylor@enterprisedb.com> writes: > > On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres@anarazel.de> wrote: > >> I think it'd also be needed in hstore_plperl.c, jsonb_plperl.c. Putting the > >> include in plperl.h would keep that aspect transparent, because plperl_utils.h > >> includes plperl.h. > > > Since plperl_helpers.h already includes plperl.h, I'm not sure why > > both are included everywhere the former is. If .c/.xs files didn't > > include plperl.h directly, we could keep pg_wchar.h in > > plperl_helpers.h. Not sure if that's workable or any better... > > Maybe we should flush the separate plperl_helpers.h header and just > put those static-inline functions in plperl.h. +1
Hi, On 2022-08-27 12:53:24 +0700, John Naylor wrote: > On Sat, Aug 27, 2022 at 11:20 AM John Naylor > <john.naylor@enterprisedb.com> wrote: > > > > Here's a patch with that idea, not tested on Windows yet. > > Update: I tried taking the CI for a spin, but ran into IT issues with > Github when I tried to push my branch to remote. A github, not a CI issue? Just making sure... As a workaround you can just open a CF entry, that'll run the patch soon. But either way, I ran the patch "manually" in a windows VM that I had running anyway. With the meson patchset, but I don't see how it could matter here. 1/5 postgresql:setup / tmp_install OK 1.30s 2/5 postgresql:jsonb_plperl / jsonb_plperl/regress OK 8.30s 3/5 postgresql:bool_plperl / bool_plperl/regress OK 8.30s 4/5 postgresql:hstore_plperl / hstore_plperl/regress OK 8.64s 5/5 postgresql:plperl / plperl/regress OK 10.41s Ok: 5 I didn't test other platforms. WRT the patch's commit message: The issue isn't that perl's free() is redefined, it's that perl's #define free (which references perl globals!) breaks windows' header... Greetings, Andres Freund
On Sat, Aug 27, 2022 at 2:23 PM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2022-08-27 12:53:24 +0700, John Naylor wrote: > > Update: I tried taking the CI for a spin, but ran into IT issues with > > Github when I tried to push my branch to remote. > > A github, not a CI issue? Just making sure... Yeah, I forked PG from the Github page, cloned it locally, applied the patch and tried to push to origin. > As a workaround you can just open a CF entry, that'll run the patch soon. Yeah, I did that after taking a break -- there are compiler warnings for contrib/sepgsql/label.c where pfree's argument is cast to void *, so seems unrelated. > But either way, I ran the patch "manually" in a windows VM that I had running > anyway. With the meson patchset, but I don't see how it could matter here. > > 1/5 postgresql:setup / tmp_install OK 1.30s > 2/5 postgresql:jsonb_plperl / jsonb_plperl/regress OK 8.30s > 3/5 postgresql:bool_plperl / bool_plperl/regress OK 8.30s > 4/5 postgresql:hstore_plperl / hstore_plperl/regress OK 8.64s > 5/5 postgresql:plperl / plperl/regress OK 10.41s > > Ok: 5 > > > I didn't test other platforms. > > > WRT the patch's commit message: The issue isn't that perl's free() is > redefined, it's that perl's #define free (which references perl globals!) > breaks windows' header... Ah, thanks for that detail and for testing, will push. -- John Naylor EDB: http://www.enterprisedb.com