windows cfbot failing: my_perl

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: 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.

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

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