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: 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(at)anarazel(dot)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(at)anarazel(dot)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(at)anarazel(dot)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(at)anarazel(dot)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(at)anarazel(dot)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(at)anarazel(dot)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(dot)naylor(at)enterprisedb(dot)com> writes:
> On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres(at)anarazel(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> John Naylor <john(dot)naylor(at)enterprisedb(dot)com> writes:
> > On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres(at)anarazel(dot)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(dot)naylor(at)enterprisedb(dot)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(dot)naylor(at)enterprisedb(dot)com> writes:
> > On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres(at)anarazel(dot)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(dot)naylor(at)enterprisedb(dot)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(at)anarazel(dot)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