Subject: Re: [PATCH] Add native windows on arm64 support

From: Thomas Munro <[email-masked]>
Date: Wed, 23 Mar 2022 16:30:58 +1300

On Tue, Mar 22, 2022 at 11:30 PM Julien Rouhaud <[email-masked]> wrote:
> On Tue, Mar 22, 2022 at 09:37:46AM +0000, Niyas Sait wrote:
> > Yes, we could look into providing a build machine. Do you have any
> > reference to what the CI system looks like now for PostgresSQL and how to
> > add new workers etc.?
>
> It's all documented at
> https://wiki.postgresql.org/wiki/PostgreSQL_Buildfarm_Howto.

It seems likely there will be more and more Windows/ARM users, so yeah
having a machine to test that combination would be great.  I wonder if
ASLR isn't breaking for you by good luck only...

---

From: Andres Freund <[email-masked]>
Date: Wed, 23 Mar 2022 18:15:31 -0700

Hi,

> ASLR isn't breaking for you by good luck only...

I think we've generally seen the ASLR issue become less prominent on
windows. Whether that's because of the silent retries we added, or because
just about everyone moved to 64bit windows / PG, I don't know. I'd guess both,
with 64bit being the larger influence.

Wonder if it's worth adding some debug logging to the retry code and stop
disabling ASLR on 64bit windows... It's imo pretty crazy that we loop up to
100 times in internal_forkexec() around CreateProcess() &&
pgwin32_ReserveSharedMemoryRegion() without, as far as I can see, a single
debug message.

I don't think we can infer too much about the failure rate on windows from the
!windows EXEC_BACKEND rates. The two internal_forkexec() implementations
behaves just too differently.

Greetings,

Andres Freund

---

From: Thomas Munro <[email-masked]>
Date: Fri, 25 Mar 2022 11:38:44 +1300

> pgwin32_ReserveSharedMemoryRegion() without, as far as I can see, a single

Yeah.  I think we should commit this patch, but decree that
Windows/aarch64 support is experimental only for now.  That allows a
build farm animal to be set up.  Then we add a bit of extra logging
and see how it does running our test suite over time and learn more.

---

From: Michael Paquier <[email-masked]>
Date: Thu, 7 Apr 2022 15:36:19 +0900

> and see how it does running our test suite over time and learn more.

I don't have such a setup so my testing capabilities are limited.
Does anybody have one?  I think that we could be flexible for this
patch, even after feature freeze as it introduces something entirely
new without impacting the existing code.  The patch has been moved to
the next CF for now.
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Thu, 07 Apr 2022 13:42:49 -0400

Andres Freund <[email-masked]> writes:
> On 2022-04-07 09:40:43 -0400, Tom Lane wrote:
>> If it causes problems down the road, how will we debug it?

> If what causes problems down the road? Afaics the patch doesn't change
> anything outside of windows-on-arm, so it shouldn't cause any breakage we care
> about until we get a buildfarm animal.

Do we have a volunteer to run a buildfarm animal?  I don't see much
point in thinking about this till there is one ready to go.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Thu, 7 Apr 2022 10:53:59 -0700

Hi,

> point in thinking about this till there is one ready to go.

OP said that they might be able to:
https://www.postgresql.org/message-id/CAFPTBD_NZb%3Dq_6uE-QV%2BS%2Bpm%3DRc9sBKrg8CQ_Y3dc27Awrm7cQ%40mail.gmail.com

Niyas, any updates on that?

Greetings,

Andres Freund

---

From: Niyas Sait <[email-masked]>
Date: Tue, 19 Apr 2022 15:22:30 +0100

> Niyas, any updates on that?

Sorry for the delay! Configuring the scripts took some time. I have
successfully run the builfarm animal script using my git repository [1]
which contains the proposed patch on a Windows Arm64 machine.

I made a request to add a new machine to build farm through [2].

I believe we should be able to enable the build farm machine once the
change has been merged.

Thanks,
Niyas

[1] https://github.com/nsait-linaro/postgres.git
[2] https://buildfarm.postgresql.org/cgi-bin/register-form.pl

---

From: Michael Paquier <[email-masked]>
Date: Wed, 20 Apr 2022 09:28:09 +0900

> I made a request to add a new machine to build farm through [2].

Have you tested with the amount of coverage provided by vcregress.pl?

Another thing I was wondering about is if it would be possible to have
this option in Travis, but that does not seem to be the case:
https://docs.travis-ci.com/user/reference/windows/#windows-version

> I believe we should be able to enable the build farm machine once the

Better to wait for the beginning of the development cycle at this
stage, based on all the replies received.  That would bring us to the
beginning of July.

+               if ($solution->{platform} eq 'ARM64') {
+                       push(@pgportfiles, 'pg_crc32c_armv8_choose.c');
+                       push(@pgportfiles, 'pg_crc32c_armv8.c');
+               } else {
+                       push(@pgportfiles, 'pg_crc32c_sse42_choose.c');
+                       push(@pgportfiles, 'pg_crc32c_sse42.c');
+               }

+++ b/src/port/pg_crc32c_armv8.c
+#ifndef _MSC_VER
 #include <arm_acle.h>
+#endif
[ ... ]
+#ifdef _M_ARM64
+   /*
+    * arm64 way of hinting processor for spin loops optimisations
+    * ref: https://community.arm.com/support-forums/f/infrastructure-solutions-forum/48654/ssetoneon-faq
+    */
+   __isb(_ARM64_BARRIER_SY);
+#else
I think that such extra optimizations had better be in a separate
patch, and we should focus on getting the build done first.

+   # arm64 linker only supports dynamic base address
+   my $cfgrandbaseaddress = $self->{platform} eq 'ARM64' ? 'true' : 'false';
This issue is still lying around, and you may have been lucky.  Would
there be any issues to remove this change to get a basic support in?
As mentioned upthread, there is a long history of Postgres with ASLR.

This would mean that a basic patch could be done with just the changes
for gendef.pl, and the first part of the changes inMSBuildProject.pm.
Would that not be enough?
--
Michael

---

From: Niyas Sait <[email-masked]>
Date: Wed, 20 Apr 2022 10:43:06 +0100

> Have you tested with the amount of coverage provided by vcregress.pl?

I built and ran the relevant tests with the help of run_build.pl.

I think following tests are executed - check, contribcheck, ecpgcheck,
installcheck, isolationcheck, modulescheck, and upgradecheck.

> https://docs.travis-ci.com/user/reference/windows/#windows-version

Yes, I think Travis doesn't yet support Windows/Arm64 platform.

> +    * ref:
https://community.arm.com/support-forums/f/infrastructure-solutions-forum/48654/ssetoneon-faq

> Would that not be enough?

I cannot build without any of the above changes. Nothing specific for
optimization
is added to this patch.

> As mentioned upthread, there is a long history of Postgres with ASLR.

MSVC linker doesn't allow non-random base addresses for Arm64 platforms.
It is needed for basic support.

Niyas

---

From: Michael Paquier <[email-masked]>
Date: Thu, 21 Apr 2022 13:07:18 +0900

> It is needed for basic support.

Why does it not allow that?  Is that just a limitation of the
compiler?  If yes, what is the error happening?  This question is not
exactly answered yet as of this thread.  I may be missing a reference
about that in the upstream docs, but I see nowhere an explanation
about the reason and the why.  That's also one of the first questions
from Thomas upthread.
--
Michael

---

From: Niyas Sait <[email-masked]>
Date: Thu, 21 Apr 2022 10:21:04 +0100

> about the reason and the why.  That's also one of the first questions

The following error occurs:

LINK : fatal error LNK1246: '/DYNAMICBASE:NO' not compatible with 'ARM64'
target machine; link without '/DYNAMICBASE:NO

This seems to be a deliberate restriction for Arm64 targets. However, no
references were provided. To clarify, I have posted a question [1] on the
community channel of Visual Studio.

Niyas

[1]
https://developercommunity.visualstudio.com/t/LINK-:-fatal-error-LNK1246:-DYNAMICBAS/10020163

---

From: Michael Paquier <[email-masked]>
Date: Mon, 25 Apr 2022 10:17:16 +0900

> target machine; link without '/DYNAMICBASE:NO

Okay, that's interesting.  In light of things like 7f3e17b, that may
be annoying.  Perhaps newer Windows versions are able to handle that
better.  I am wondering if it would be worth re-evaluating this
change, and attempt to re-enable that in environments other than
arm64.  This could become interesting if we consider that there have
been talks to cut the support for a bunch of Windows versions to focus
on the newer ones only.

> community channel of Visual Studio.

Thanks for doing that!  Your post is just a couple of days old, let's
see if we get a reply on that.
--
Michael

---

From: Niyas Sait <[email-masked]>
Date: Mon, 9 May 2022 12:44:22 +0100

> see if we get a reply on that.

Microsoft updated documentation [1] and clarified that ASLR cannot be
disabled for Arm64 targets.

Because ASLR can't be disabled on ARM, ARM64, or ARM64EC architectures,
> /DYNAMICBASE:NO isn't supported for these targets.

Thanks
Niyas

[1]
https://docs.microsoft.com/en-us/cpp/build/reference/dynamicbase?view=msvc-170

---

From: Michael Paquier <[email-masked]>
Date: Tue, 10 May 2022 10:01:55 +0900

> > /DYNAMICBASE:NO isn't supported for these targets.

Better than nothing, I guess, though this does not stand as a reason
explaining why this choice has been made.  And it looks like we will
never know.  We are going to need more advanced testing to check that
DYNAMICBASE is stable enough if we begin to enable it.  Perhaps we
should really look at that for all the build targets we currently
support rather than just ARM, for the most modern Win32 environments
if we are going to cut support for most of the oldest releases..
--
Michael

---

From: Niyas Sait <[email-masked]>
Date: Thu, 14 Jul 2022 09:36:13 +0100

Hello,

Please find a new patch (no further changes) rebased on top of the master.

-- 
Niyas Sait
Linaro Limited

---

From: Andres Freund <[email-masked]>
Date: Thu, 25 Aug 2022 18:29:07 -0700

Hi,

> if we are going to cut support for most of the oldest releases..

I accidentally did a lot of testing with DYNAMICBASE - I accidentally
mistranslated MSBuildProject.pm's <RandomizedBaseAddress>false</> to adding
/DYNAMICBASE in the meson port. Survived several hundred CI cycles and dozens
of local runs without observing any problems related to that.

Which doesn't surprise me, given the way we reserve space in the new process.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Fri, 26 Aug 2022 11:09:41 +0900

> Which doesn't surprise me, given the way we reserve space in the new process.

Still, we had problems with that in the past and I don't recall huge
changes in the way we allocate shmem on WIN32.  Could there be some
stuff in Windows itself that would explain more stability?  I would
not mind switching back to DYNAMICBASE on HEAD seeing your results.
That would simplify this thread's patch a bit as well.
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Thu, 25 Aug 2022 20:29:43 -0700

Hi,

On 2022-08-26 11:09:41 +0900, Michael Paquier wrote:
> changes in the way we allocate shmem on WIN32.

It's really not great that 7f3e17b4827 disabled randomization without even a
comment as to why...

There actually seems to have been a lot of work in that area. See 617dc6d299c
/ bcbf2346d69 and as well as the retrying in 45e004fb4e39. Those weren't
prevented by us disabling ASLR.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Sat, 27 Aug 2022 11:33:51 +0900

> It's really not great that 7f3e17b4827 disabled randomization without even a

This story is on this thread, with some processes not able to start:
https://www.postgresql.org/message-id/BD0D89EC2438455C9DE0DC94D36912F4@maumau

> prevented by us disabling ASLR.

Indeed.  45e004f looks like the most interesting bit here.  FWIW, I
would not mind re-enabling that on HEAD, as of something like the
attached.  I have done a dozen of runs without seeing a test failure,
and knowing that we don't support anything older than Win10 makes me
feel safer about this change.  Any objections?
--
Michael
From 0fde47c2d4e65bd9f1f5a7b4607f8b8b95162242 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Sat, 27 Aug 2022 10:20:53 +0900
Subject: [PATCH] Re-enable ASLR on Windows

---
 src/tools/msvc/MSBuildProject.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
index 62acdda3a1..594729ceb7 100644
--- a/src/tools/msvc/MSBuildProject.pm
+++ b/src/tools/msvc/MSBuildProject.pm
@@ -347,7 +347,6 @@ sub WriteItemDefinitionGroup
       <ProgramDatabaseFile>.\\$cfgname\\$self->{name}\\$self->{name}.pdb</ProgramDatabaseFile>
       <GenerateMapFile>false</GenerateMapFile>
       <MapFileName>.\\$cfgname\\$self->{name}\\$self->{name}.map</MapFileName>
-      <RandomizedBaseAddress>false</RandomizedBaseAddress>
       <!-- Permit links to MinGW-built, 32-bit DLLs (default before VS2012). -->
       <ImageHasSafeExceptionHandlers/>
       <SubSystem>Console</SubSystem>
-- 
2.37.2

---

From: Tom Lane <[email-masked]>
Date: Fri, 26 Aug 2022 23:02:58 -0400

> feel safer about this change.  Any objections?

We're early enough in the v16 cycle to have plenty of time to detect
any problems, so I see little reason not to try it.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Sat, 27 Aug 2022 12:27:57 -0700

Hi,

On 2022-08-27 11:33:51 +0900, Michael Paquier wrote:
> FWIW, I would not mind re-enabling that on HEAD, as of something like the
> attached.  I have done a dozen of runs without seeing a test failure, and
> knowing that we don't support anything older than Win10 makes me feel safer
> about this change.  Any objections?

+1. We don't have a choice about it on other architectures and it seems like a
bad idea to disable on its own.

I checked, and it looks like we didn't add --disable-dynamicbase, so ASLR
wasn't disabled for mingw builds.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Sun, 28 Aug 2022 16:23:08 +0900

> wasn't disabled for mingw builds.

True enough.  I have applied the patch to re-enable that on HEAD.
Let's now see what happens in the next couple of days.  Popcorn is
ready here.
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Sun, 28 Aug 2022 10:09:53 -0400

> Let's now see what happens in the next couple of days.  Popcorn is

Hmm:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-28%2010%3A30%3A29

Maybe that's just unrelated noise, but it sure looks weird: after
passing the core regression tests in "make check", it failed in
pg_upgrade's run with

diff -w -U3 H:/prog/bf/root/HEAD/pgsql.build/src/test/regress/expected/xml.out H:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/results/xml.out
--- H:/prog/bf/root/HEAD/pgsql.build/src/test/regress/expected/xml.out	Wed May 18 18:30:12 2022
+++ H:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/results/xml.out	Sun Aug 28 06:55:14 2022
@@ -1557,7 +1557,7 @@
 \\x
 SELECT * FROM XMLTABLE('*' PASSING '<e>pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post</e>' COLUMNS x xml PATH 'node()', y xml PATH '/');
 -[ RECORD 1 ]-----------------------------------------------------------
-x | pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post
+x | pre<?pi arg?><![CDATA[&ent1]]><!--c1--><n2>&amp;deep</n2>post
 y | <e>pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post</e>+
   | 

I don't recall ever seeing a failure quite like this.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Sun, 28 Aug 2022 08:41:54 -0700

Hi,

> +++ H:/prog/bf/root/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/results/xml.out	Sun Aug 28 06:55:14 2022

Pretty weird, agreed. But hard to see how it could be caused by the
randomization change, except that perhaps it could highlight a preexisting
bug?

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Sun, 28 Aug 2022 11:51:19 -0400

> randomization change, except that perhaps it could highlight a preexisting

I have no idea either.  I agree there *shouldn't* be any connection,
so if ASLR is somehow triggering this then whatever is failing is
almost certainly buggy on its own terms.  But there's a lot of
moving parts here (mumble libxml mumble).  I'm going to wait to see
if it reproduces before spending much effort.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Mon, 29 Aug 2022 10:12:20 +0900

> if it reproduces before spending much effort.

I have noticed that yesterday, but cannot think much about it.  This
basically changes the position of "<!--c1-->" for the first record,
leaving the second one untouched:
<!--c1--><?pi arg?><![CDATA[&ent1]]>
<?pi arg?><![CDATA[&ent1]]><!--c1-->

I am not used to xmltable(), but I wonder if there is something in one
of these support functions in xml.c that gets influenced by the
randomization.  That sounds a bit hairy as make check passed in
bowerbird, and I have noticed at least two other Windows hosts running
TAP that passed.  Or that's just something with libxml itself.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 30 Aug 2022 08:48:38 +0900

> TAP that passed.  Or that's just something with libxml itself.

This is amazing.  The issue has showed up a second time in a row in
bowerbird, as of:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-29%2013%3A30%3A32

I don't know what to think about ASLR that manipulates the comment in
this XML object under VS 2017 (perhaps a compiler issue?), but it
should be possible to go back to green simply by removing "<!--c1-->"
from the input string.  Creating an extra output pattern here would be
very costly, as xml_2.out and xml.out have outputs for --with-libxml.

Would people object if I do that for now?
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Mon, 29 Aug 2022 19:58:31 -0400

> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-29%2013%3A30%3A32

That is fascinating.

> Would people object if I do that for now?

Let's let it go for a few more runs.  I want to know whether it
reproduces 100% or only sometimes.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Mon, 29 Aug 2022 17:09:54 -0700

Hi,

> Would people object if I do that for now?

I do. Clearly something is wrong, what do we gain by averting our eyes? An
alternative output file strikes me as an equally bad idea - it's not like this
is a case where there are legitimately different outputs?

Bowerbird is the only animal testing libxml with visual studio afaics. And CI
doesn't have libxml either.  So we have no idea if it's the msvc version that
matters.

Andrew, what version of libxml is installed? Afaict we don't see that anywhere
with msvc.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Mon, 29 Aug 2022 17:33:56 -0700

Hi,

> reproduces 100% or only sometimes.

The weirdest part is that it only happens as part of the the pg_upgrade test.

I just tested it in my test windows vm (win 10, vs 2019), with a build of
libxml I had around (2.9.7), and the regression tests passed both "normally"
and within pg_upgrade.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Tue, 30 Aug 2022 10:35:35 +0900

> The weirdest part is that it only happens as part of the the pg_upgrade test.

make check has just failed:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-30%2001%3A15%3A13

> libxml I had around (2.9.7), and the regression tests passed both "normally"

bowerbird is feeling from c:\\prog\\3p64\\include\\libxml2 and
c:\\prog\\3p64\\lib\\libxml2.lib.  I am not sure which version of
libxml this is, and the other animals of MSVC don't use libxml so it
is not possible to correlate that only to VS 2017, either.
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Mon, 29 Aug 2022 22:00:10 -0400

> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2022-08-30%2001%3A15%3A13

So it *is* probabilistic, which is pretty much what you'd expect
if ASLR triggers it.  That brings us no closer to understanding
what the mechanism is, though.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Wed, 31 Aug 2022 08:36:01 +0900

> what the mechanism is, though.

There have been more failures, always switching the input from
"pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post"
to "pre<?pi arg?><![CDATA[&ent1]]><!--c1--><n2>&amp;deep</n2>post".

Using a PATH of node() influences the output.  I am not verse unto
XMLTABLE, but could it be an issue where each node is parsed and we
have something like a qsort() applied on the pointer addresses for
each part or something like that, causing the output to become
unstable?
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Tue, 30 Aug 2022 20:07:27 -0400

> each part or something like that, causing the output to become

I'm not sure.  I dug into this enough to find that the output from
this query is generated in xml.c lines 4635ff:

                    /* Concatenate serialized values */
                    initStringInfo(&str);
                    for (int i = 0; i < count; i++)
                    {
                        textstr =
                            xml_xmlnodetoxmltype(xpathobj->nodesetval->nodeTab[i],
                                                 xtCxt->xmlerrcxt);

                        appendStringInfoText(&str, textstr);
                    }
                    cstr = str.data;

So evidently, the problem occurs because the elements of the
nodesetval->nodeTab[] array are in a different order than we expect.

I looked into <xpath.h> and found the definition of the "nodesetval"
struct, and the comments are eye-opening to say the least:

/*
 * A node-set (an unordered collection of nodes without duplicates).
 */
typedef struct _xmlNodeSet xmlNodeSet;
typedef xmlNodeSet *xmlNodeSetPtr;
struct _xmlNodeSet {
    int nodeNr;			/* number of nodes in the set */
    int nodeMax;		/* size of the array as allocated */
    xmlNodePtr *nodeTab;	/* array of nodes in no particular order */
    /* @@ with_ns to check wether namespace nodes should be looked at @@ */
};

It seems like maybe we're relying on an ordering we should not.
Yet surely the ordering of the pieces of this output is meaningful?
Are we using the wrong libxml API to create this result?

Anyway, I'm now suspicious that we've accidentally exposed a logic
bug in the XMLTABLE code, rather than anything wrong with the
ASLR stuff.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Wed, 31 Aug 2022 09:15:00 +0900

> each part or something like that, causing the output to become

Hmm.  I think that I may have an idea here after looking at our xml.c
and xpath.c in libxml2/.  From what I understand, we process the PATH
through XmlTableGetValue() that builds a XML node path in
xmlXPathCompiledEval().  The interesting part comes from libxml2's
xmlXPathCompiledEvalInternal(), where I think we don't apply a sort on
the contents generated.  Hence, I am wondering if the solution here
would be to do one xmlXPathNodeSetSort(xpathobj->nodesetval) after
compiling the path with xmlXPathCompiledEval() in XmlTableGetValue().
This should ensure that the items are ordered even if ASLR mixes if
the pointer positions.

A complete solution would involve more code paths, but we'd need only
one change in XmlTableGetValue() for the current regression tests to
work.  I don't have an environment where I can reproduce that, so that
would be up to the buildfarm to stress this solution..

Thoughts?
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Wed, 31 Aug 2022 09:16:50 +0900

> bug in the XMLTABLE code, rather than anything wrong with the

Funny.  I have reached basically the same conclusion as you a couple
of minutes ago, but I also think that I have found what we need to do
here to ensure the ordering of the nodes generated by the libxml
code.
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Tue, 30 Aug 2022 20:28:58 -0400

Michael Paquier <[email-masked]> writes:
> Hence, I am wondering if the solution here

Hmm, I see that function declared in <xpathInternals.h>, which
sure doesn't give me a warm feeling that we're supposed to call it
directly.  But maybe there's an approved way to get the result.

Or perhaps this test case is wrong, and instead of "node()" we
need to write something that specifies a sorted result?

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Wed, 31 Aug 2022 15:09:16 +0900

> need to write something that specifies a sorted result?

I am no specialist on this matter..  Still, looking around, this looks
to be the kind of job that xsl:sort would do?  We cannot use it here
for an XML-only context, though, and it does not seem like there is a
direct way to enforce the ordering per the nature of the XPath
language, either.  I may be missing something of course.

Please note that I am also getting a bit the cold feet with the idea
of enforcing a sorting of the elements in the generated output,
actually, as this could have a performance penalty for all users
particularly on large blobs of data.

At the end, I'd like to think that it would be wiser to just remove
entirely the test for node() or reduce the contents of the string to
be able to strictly control the output order (say a simple
'<e>pre<![CDATA[&ent1]]>post</e>' would do the trick).  I cannot think
about a better idea now.  Note that removing the test case where we
have node() has no impact on the coverage of xml.c.
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Wed, 31 Aug 2022 13:33:53 -0400

> have node() has no impact on the coverage of xml.c.

Yeah, I confirm that: local code-coverage testing shows no change
in the number of lines reached in xml.c when I remove that column:

-SELECT * FROM XMLTABLE('*' PASSING '<e>pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post</e>' COLUMNS x xml PATH 'node()', y xml PATH '/');
+SELECT * FROM XMLTABLE('*' PASSING '<e>pre<!--c1--><?pi arg?><![CDATA[&ent1]]><n2>&amp;deep</n2>post</e>' COLUMNS y xml PATH '/');

Dropping the query altogether does result in a reduction in the
number of lines hit.

I did not check the other XMLTABLE infrastructure, so what probably
is best to do is keep the two output columns but change 'node()'
to something with a more stable result; any preferences?

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Thu, 1 Sep 2022 09:32:38 +0900

> to something with a more stable result; any preferences?

The input object could also be reworked so as we would not have any
ordering issues, say "<e>pre<f><g><n2>&amp;deep</n2></g></f>post</e>".
Changing only the path, you could use "/e/n2" instead of "node()", so
as we'd always get the most internal member.
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Wed, 31 Aug 2022 22:22:06 -0400

> as we'd always get the most internal member.

Done that way.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Thu, 1 Sep 2022 13:41:59 +0900

> > as we'd always get the most internal member.

Cool, thanks.  bowerbird looks happy after its first run.

An argument in favor of backpatching is if one decides to build the
code with MSVC and patch the scripts to enable ASLR.  However, nobody
has complained about that yet, so fine by me to leave this change only
on HEAD for now.
--
Michael

---

From: Ian Lawrence Barwick <[email-masked]>
Date: Thu, 3 Nov 2022 17:38:09 +0900

> has complained about that yet, so fine by me to leave this change only

Apologies for the thread bump, but there is an entry for this thread
in the current CF
previously marked "Needs review":

    https://commitfest.postgresql.org/40/3561/

I've gone ahead and marked it as "Committed", as that appears to have happened
back in August as 36389a060c.

If for whatever reason that was the Wrong Thing To Do, please let me know.

Regards

Ian Barwick

---

From: Niyas Sait <[email-masked]>
Date: Thu, 3 Nov 2022 11:06:46 +0000

> If for whatever reason that was the Wrong Thing To Do, please let me know.

36389a060c commit enables ASLR for windows but does not include other
changes required for Arm64.

I've attached a new version of the patch which excludes the already merged
ASLR changes and add
small changes to handle latest changes in the build scripts.

On Thu, 3 Nov 2022 at 08:38, Ian Lawrence Barwick <[email-masked]> wrote:

> > > > ordering issues, say
> "<e>pre<f><g><n2>&amp;deep</n2></g></f>post</e>".

-- 
Niyas Sait
Linaro Limited

---

From: Michael Paquier <[email-masked]>
Date: Thu, 3 Nov 2022 20:43:04 +0900

> changes required for Arm64.

Yes, marking this patch as committed is incorrect.  That's just me
lagging behind for the review :)

I have switched that back as "Needs review" for the moment.

> small changes to handle latest changes in the build scripts.

Thanks.
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Sat, 5 Nov 2022 11:31:36 -0700

Hi,

> small changes to handle latest changes in the build scripts.

Note that we're planning to remove the custom windows build scripts before the
next release, relying on the meson build instead.

> diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
> index 8b19ab160f..bf6a6dba35 100644
> --- a/src/include/storage/s_lock.h
> +++ b/src/include/storage/s_lock.h
> @@ -708,13 +708,21 @@ typedef LONG slock_t;
>  #define SPIN_DELAY() spin_delay()
>  
>  /* If using Visual C++ on Win64, inline assembly is unavailable.
> - * Use a _mm_pause intrinsic instead of rep nop.
> + * Use _mm_pause (x64) or __isb(arm64) intrinsic instead of rep nop.
>   */
>  #if defined(_WIN64)
>  static __forceinline void
>  spin_delay(void)
>  {
> +#ifdef _M_ARM64
> +	/*
>  	_mm_pause();
>  #else
>  static __forceinline void

I think we should just apply this, there seems very little risk of making
anything worse, given the gating to _WIN64 && _M_ARM64.

> diff --git a/src/port/pg_crc32c_armv8.c b/src/port/pg_crc32c_armv8.c
> index 9e301f96f6..981718752f 100644
> --- a/src/port/pg_crc32c_armv8.c
> @@ -14,7 +14,9 @@
>  #include "c.h"
>  #include "port/pg_crc32c.h"

This won't suffice with the meson build, since the relevant configure test
also uses arm_acle.h:
elif host_cpu == 'arm' or host_cpu == 'aarch64'

  prog = '''
#include <arm_acle.h>

int main(void)
{
    unsigned int crc = 0;
    crc = __crc32cb(crc, 0);
    crc = __crc32ch(crc, 0);
    crc = __crc32cw(crc, 0);
    crc = __crc32cd(crc, 0);

    /* return computed value, to prevent the above being optimized away */
    return crc == 0;
}
'''

  if cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd without -march=armv8-a+crc',
      args: test_c_args)
    # Use ARM CRC Extension unconditionally
    cdata.set('USE_ARMV8_CRC32C', 1)
    have_optimized_crc = true
  elif cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd with -march=armv8-a+crc',
      args: test_c_args + ['-march=armv8-a+crc'])
    # Use ARM CRC Extension, with runtime check
    cflags_crc += '-march=armv8-a+crc'
    cdata.set('USE_ARMV8_CRC32C', false)
    cdata.set('USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK', 1)
    have_optimized_crc = true
  endif
endif

The meson checking logic is used both for msvc and other compilers, so this
will need to work with both.

> diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
> index d6bed1ce15..f1e0ff446b 100644
> --- a/src/tools/msvc/gendef.pl
> +++ b/src/tools/msvc/gendef.pl
> @@ -120,9 +120,9 @@ sub writedef
>  	{
>  		my $isdata = $def->{$f} eq 'data';
>  
> -		# Strip the leading underscore for win32, but not x64
> +		# Strip the leading underscore for win32, but not x64 and arm64
>  		$f =~ s/^_//
> -		  unless ($arch eq "x86_64");
> +		  unless ($arch ne "x86");
>  
>  		# Emit just the name if it's a function symbol, or emit the name
>  		# decorated with the DATA option for variables.
> @@ -143,7 +143,7 @@ sub writedef
>  sub usage
>  {
>  	die("Usage: gendef.pl --arch <arch> --deffile <deffile> --tempdir <tempdir> files-or-directories\n"
> -		  . "    arch: x86 | x86_64\n"
> +		  . "    arch: x86 | x86_64 | arm64 \n"
>  		  . "    deffile: path of the generated file\n"
>  		  . "    tempdir: directory for temporary files\n"
>  		  . "    files or directories: object files or directory containing object files\n"
> @@ -160,7 +160,7 @@ GetOptions(
>  	'tempdir:s' => \$tempdir,) or usage();
>  
>  usage("arch: $arch")
> -  unless ($arch eq 'x86' || $arch eq 'x86_64');
> +  unless ($arch eq 'x86' || $arch eq 'x86_64' || $arch eq 'arm64');
>  
>  my @files;
>  

Seems reasonable.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Mon, 7 Nov 2022 15:58:13 +0900

> next release, relying on the meson build instead.

Youpi.

> anything worse, given the gating to _WIN64 && _M_ARM64.

Seems so.  Hmm, where does _ARM64_BARRIER_SY come from?  Perhaps it
would be better to have a comment referring to it from a different
place than the forums of arm, like some actual docs?

> will need to work with both.

The patch has been switched as waiting on author for now.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Thu, 1 Dec 2022 16:42:19 +0900

> The patch has been switched as waiting on author for now.

Seeing no replies after three weeks, I have marked the patch as
returned with feedback for now.
--
Michael

---

From: Niyas Sait <[email-masked]>
Date: Thu, 1 Dec 2022 17:17:53 +0000

> next release, relying on the meson build instead.

Thanks. I will add changes to add meson build support.

> will need to work with both.

Yes, will handle that.

-- 
Niyas

---

From: Niyas Sait <[email-masked]>
Date: Thu, 1 Dec 2022 17:20:20 +0000

> place than the forums of arm, like some actual docs?

_ARM64_BARRIER_SY is defined in Microsoft Arm64 intrinsic documentation 
- 
https://learn.microsoft.com/en-us/cpp/intrinsics/arm64-intrinsics?view=msvc-170#BarrierRestrictions

I couldn't find something more official for the sse2neon library part.

-- 
Niyas

---

From: Niyas Sait <[email-masked]>
Date: Thu, 1 Dec 2022 17:53:47 +0000

Hello,

I've attached a new revision of the patch (v4) and includes following 
changes,

1. Add support for meson build system
2. Extend MSVC scripts to handle ARM64 platform.
3. Add arm64 definition of spin_delay function.
4. Exclude arm_acle.h import with MSVC compiler.

V3->V4: Add support for meson build system
V2->V3: Removed ASLR enablement and rebased on master.
V1->V2: Rebased on top of master

-- 
Niyas

From 872f538f6261b36a32cbcecae82e99778b747799 Mon Sep 17 00:00:00 2001
From: Niyas Sait <[email-masked]>
Date: Tue, 22 Feb 2022 13:07:24 +0000
Subject: [PATCH v4] Enable postgres native build for windows-arm64 platform

Following changes are included

- Extend MSVC scripts to handle ARM64 platform
- Add arm64 definition of spin_delay function
- Exclude arm_acle.h import for MSVC
- Add support for meson build
---
 meson.build                      | 33 +++++++++++++++++++-------------
 src/include/storage/s_lock.h     | 10 +++++++++-
 src/port/pg_crc32c_armv8.c       |  2 ++
 src/tools/msvc/MSBuildProject.pm | 16 ++++++++++++----
 src/tools/msvc/Mkvcbuild.pm      |  9 +++++++--
 src/tools/msvc/Solution.pm       | 11 ++++++++---
 src/tools/msvc/gendef.pl         |  8 ++++----
 7 files changed, 62 insertions(+), 27 deletions(-)

diff --git a/meson.build b/meson.build
index 725e10d815..e354ad7650 100644
--- a/meson.build
+++ b/meson.build
@@ -1944,7 +1944,13 @@ int main(void)

-  prog = '''
+  if cc.get_id() == 'msvc'
+    cdata.set('USE_ARMV8_CRC32C', false)
+    cdata.set('USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK', 1)
+    have_optimized_crc = true
+  else
+
+    prog = '''
 #include <arm_acle.h>
 
 int main(void)
@@ -1960,18 +1966,19 @@ int main(void)
 }
 '''
 
-  if cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd without -march=armv8-a+crc',
-      args: test_c_args)
-    # Use ARM CRC Extension unconditionally
-    cdata.set('USE_ARMV8_CRC32C', 1)
-    have_optimized_crc = true
-  elif cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd with -march=armv8-a+crc',
-      args: test_c_args + ['-march=armv8-a+crc'])
-    # Use ARM CRC Extension, with runtime check
-    cflags_crc += '-march=armv8-a+crc'
-    cdata.set('USE_ARMV8_CRC32C', false)
-    cdata.set('USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK', 1)
-    have_optimized_crc = true
+    if cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd without -march=armv8-a+crc',
+        args: test_c_args)
+      # Use ARM CRC Extension unconditionally
+      cdata.set('USE_ARMV8_CRC32C', 1)
+      have_optimized_crc = true
+    elif cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd with -march=armv8-a+crc',
+        args: test_c_args + ['-march=armv8-a+crc'])
+      # Use ARM CRC Extension, with runtime check
+      cflags_crc += '-march=armv8-a+crc'
+      cdata.set('USE_ARMV8_CRC32C', false)
+      cdata.set('USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK', 1)
+      have_optimized_crc = true
+    endif
   endif
 endif

 
+#ifndef _MSC_VER
 #include <arm_acle.h>
+#endif

diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
index 58590fdac2..274ddc8860 100644
--- a/src/tools/msvc/MSBuildProject.pm
+++ b/src/tools/msvc/MSBuildProject.pm
@@ -310,10 +310,18 @@ sub WriteItemDefinitionGroup
 	  : ($self->{type} eq "dll" ? 'DynamicLibrary' : 'StaticLibrary');
 	my $libs = $self->GetAdditionalLinkerDependencies($cfgname, ';');
 
-	my $targetmachine =
-	  $self->{platform} eq 'Win32' ? 'MachineX86' : 'MachineX64';
-	my $arch =
-	  $self->{platform} eq 'Win32' ? 'x86' : 'x86_64';
+	my $targetmachine;
+	my $arch;
+	if ($self->{platform} eq 'Win32') {
+		$targetmachine = 'MachineX86';
+		$arch = 'x86';
+	} elsif ($self->{platform} eq 'ARM64'){
+		$targetmachine = 'MachineARM64';
+		$arch = 'aarch64';
+	} else {
+		$targetmachine = 'MachineX64';
+		$arch = 'x86_64';
+	}
 
 	my $includes = join ';', @{ $self->{includes} }, "";
 
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 7e52e9ad0a..c6e8e91f5f 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -123,8 +123,13 @@ sub mkvcbuild
 
 	if ($vsVersion >= '9.00')
 	{
-		push(@pgportfiles, 'pg_crc32c_sse42_choose.c');
-		push(@pgportfiles, 'pg_crc32c_sse42.c');
+		if ($solution->{platform} eq 'ARM64') {
+			push(@pgportfiles, 'pg_crc32c_armv8_choose.c');
+			push(@pgportfiles, 'pg_crc32c_armv8.c');
+		} else {
+			push(@pgportfiles, 'pg_crc32c_sse42_choose.c');
+			push(@pgportfiles, 'pg_crc32c_sse42.c');
+		}
 		push(@pgportfiles, 'pg_crc32c_sb8.c');
 	}
 	else
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index c2acb58df0..891b7c5f5a 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -67,8 +67,13 @@ sub DeterminePlatform
 		# Examine CL help output to determine if we are in 32 or 64-bit mode.
 		my $output = `cl /help 2>&1`;
 		$? >> 8 == 0 or die "cl command not found";
-		$self->{platform} =
-		  ($output =~ /^\/favor:<.+AMD64/m) ? 'x64' : 'Win32';
+		if ($output =~ /^\/favor:<.+AMD64/m) {
+			$self->{platform} = 'x64';
+		} elsif($output =~ /for ARM64$/m) {
+			$self->{platform} = 'ARM64';
+		} else {
+			$self->{platform} = 'Win32';
+		}
 	}
 	else
 	{
@@ -423,7 +428,7 @@ sub GenerateFiles
 		STDC_HEADERS                        => 1,
 		STRERROR_R_INT                      => undef,
 		USE_ARMV8_CRC32C                    => undef,
-		USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK => undef,
+		USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK => 1,
 		USE_ASSERT_CHECKING => $self->{options}->{asserts} ? 1 : undef,
 		USE_BONJOUR         => undef,
 		USE_BSD_AUTH        => undef,
diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index d6bed1ce15..ad1cf86ebd 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -120,9 +120,9 @@ sub writedef
 	{
 		my $isdata = $def->{$f} eq 'data';
 
-		# Strip the leading underscore for win32, but not x64
+		# Strip the leading underscore for win32, but not x64 and aarch64
 		$f =~ s/^_//
-		  unless ($arch eq "x86_64");
+		  unless ($arch ne "x86");
 
+		  . "    arch: x86 | x86_64 | aarch64 \n"
 
 usage("arch: $arch")
-  unless ($arch eq 'x86' || $arch eq 'x86_64');
+  unless ($arch eq 'x86' || $arch eq 'x86_64' || $arch eq 'aarch64');
 
 my @files;
 
-- 
2.38.1

---

From: Michael Paquier <[email-masked]>
Date: Fri, 2 Dec 2022 14:02:54 +0900

> V1->V2: Rebased on top of master

Thanks for the updated version.  I have been looking at it closely and
it looks like it should be able to do the job (no arm64 machine for
Windows here, sigh).

I have one tiny comment about this part:

Shouldn't we only enable this flag when we are under aarch64?
Similarly, I don't think that it is a good idea to switch on
USE_SSE42_CRC32C_WITH_RUNTIME_CHECK all the time.  We should only set
it when building under x86 and x86_64.

I would also add your link [1] in s_lock.h.

[1]: https://learn.microsoft.com/en-us/cpp/intrinsics/arm64-intrinsics#BarrierRestrictions
--
Michael

---

From: John Naylor <[email-masked]>
Date: Fri, 2 Dec 2022 12:41:06 +0700

In particular, at the bottom of that section:

"For the __isb intrinsic, the only restriction that is currently valid is
_ARM64_BARRIER_SY; all other values are reserved by the architecture."

This corresponds to

https://developer.arm.com/documentation/ddi0596/2021-06/Base-Instructions/ISB--Instruction-Synchronization-Barrier-

which says

"SY Full system barrier operation, encoded as CRm = 0b1111. Can be omitted."

> I couldn't find something more official for the sse2neon library part.

Not quite sure what this is referring to, but it seems we can just point to
the __aarch64__ section in the same file, which uses the same instruction:

spin_delay(void)
{
  __asm__ __volatile__(
  " isb; \n");
}

...and which already explains the choice with a comment.

About v4:

Need a space here after __isb.

That seems like a heavy-handed way to force it. Could we just use the same
gating in the test program that the patch puts in the code of interest?
Namely:

+#ifndef _MSC_VER
 #include <arm_acle.h>
+#endif

--
John Naylor
EDB: http://www.enterprisedb.com

---

From: Niyas Sait <[email-masked]>
Date: Fri, 2 Dec 2022 09:37:43 +0000

> it when building under x86 and x86_64.

Ok, I will fix ARM64 specific one in the next revision.

-- 
Niyas

---

From: Niyas Sait <[email-masked]>
Date: Fri, 2 Dec 2022 09:50:20 +0000

> ...and which already explains the choice with a comment.

Good point. Will add the comment.

> gating in the test program that the patch puts in the code of interest?
I took a similar approach as x86 MSVC code. I don't think the test 
program would work with MSVC. The compiler options are not MSVC friendly.

-- 
Niyas

---

From: Niyas Sait <[email-masked]>
Date: Fri, 2 Dec 2022 11:09:15 +0000

Hello,

I've attached a new revision of the patch (v5) and includes following 
changes,

V4->V5: * Added reference to Microsoft arm64 intrinsic documentation
         * Conditionally enable USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK
         * Fixed styling issue spotted by Ian Lawerence Barwick
V3->V4: Add support for meson build system
V2->V3: Removed ASLR enablement and rebased on master.
V1->V2: Rebased on top of master

-- 
Niyas

From 58b82107088456fc71d239f4e1b995d91a94b4ef Mon Sep 17 00:00:00 2001
From: Niyas Sait <[email-masked]>
Date: Tue, 22 Feb 2022 13:07:24 +0000
Subject: [PATCH v5] Enable postgres native build for windows-arm64 platform

 
 
-  prog = '''
 
 int main(void)

index 8b19ab160f..ab6a6e0281 100644
 
 /* If using Visual C++ on Win64, inline assembly is unavailable.
- * Use a _mm_pause intrinsic instead of rep nop.
+ * Use _mm_pause (x64) or __isb (arm64) intrinsic instead of rep nop.
  */
 #if defined(_WIN64)
 static __forceinline void
 spin_delay(void)
 {
+#ifdef _M_ARM64
+	/*
+	 * See spin_delay aarch64 inline assembly definition above for details
+	 * ref: https://learn.microsoft.com/en-us/cpp/intrinsics/arm64-intrinsics#BarrierRestrictions
+	*/
+	__isb(_ARM64_BARRIER_SY);
+#else
 	_mm_pause();
+#endif
 }
 #else
 static __forceinline void
diff --git a/src/port/pg_crc32c_armv8.c b/src/port/pg_crc32c_armv8.c
index 9e301f96f6..981718752f 100644
--- a/src/port/pg_crc32c_armv8.c
+++ b/src/port/pg_crc32c_armv8.c
@@ -14,7 +14,9 @@
  */
 #include "c.h"
 
+#ifndef _MSC_VER
 #include <arm_acle.h>
+#endif

 
-	my $targetmachine =

 
index c2acb58df0..5eb4ccb8ff 100644
+		USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK => $self->{platform} eq "ARM64" ? : 1 : undef,

 
 usage("arch: $arch")
 
 my @files;
 
-- 
2.38.1.windows.1

---

From: Michael Paquier <[email-masked]>
Date: Mon, 5 Dec 2022 14:12:41 +0900

> 4. Exclude arm_acle.h import with MSVC compiler.

Hmm.  There are still a few things that need some attention here:
- USE_SSE42_CRC32C_WITH_RUNTIME_CHECK should not be set for aarch64.
- This is missing updates for ICU.  Looking at the upstream code,
Build.Windows.ProjectConfiguration.props uses libARM64 and binARM64
for the output library and binary paths.
- This is missing updates for krb5.  For this case, I am seeing no
traces of packages for aarch64, so I guess that we could just fail
hard until someone cares enough to ping us about what to do here.
- There were zero changes in the docs, but we need to update at least
the section about architectures supported for the 64-bit builds.
- Last comes OpenSSL, that supports amd64_arm64 as build target (see
NOTES-WINDOWS.md), and the library names are libssl.lib and
libcrypto.lib.  Looking at
https://slproweb.com/products/Win32OpenSSL.html, there are
experimental builds for arm64 with OpenSSL 3.0.  Niyas or somebody
else, could you look at the contents of lib/VC/ and see what we could
rely on for the debug builds after installing this MSI?  We should
rely on something like lib/VC/sslcrypto64MD.lib or
lib/VC/sslcrypto32MD.lib, but for arm64.

With meson gaining in maturity, perhaps that's not the most urgent
thing as we will likely remove src/tools/msvc/ soon but I'd rather do
that right anyway as much as I can to avoid an incorrect state in the
tree at any time in its history.

-       USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK => undef,
+       USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK => $self->{platform} eq "ARM64" ? : 1 : undef,
Did you actually test this patch?  This won't work at all with perl,
per se the double colon after the question mark.

For now, please find attached an updated patch with all the fixes I
could come up with.
--
Michael
From 9cb858cfdb25c3a119a8b28321cf3218eac282c5 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Mon, 5 Dec 2022 14:09:29 +0900
Subject: [PATCH v6] Enable postgres native build for windows-arm64 platform

- Extend MSVC scripts to handle ARM64 platform
- Add arm64 definition of spin_delay function
- Exclude arm_acle.h import for MSVC
- Add support for meson build
---
 src/include/storage/s_lock.h      | 10 +++++++++-
 src/port/pg_crc32c_armv8.c        |  2 ++
 doc/src/sgml/install-windows.sgml |  3 ++-
 meson.build                       | 33 +++++++++++++++++++------------
 src/tools/msvc/MSBuildProject.pm  | 16 +++++++++++----
 src/tools/msvc/Mkvcbuild.pm       |  9 +++++++--
 src/tools/msvc/Solution.pm        | 30 ++++++++++++++++++++++------
 src/tools/msvc/gendef.pl          |  8 ++++----
 8 files changed, 80 insertions(+), 31 deletions(-)

 
+#ifndef _MSC_VER
 #include <arm_acle.h>
+#endif

diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index bbd4960e7b..3f865d7d3b 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -352,7 +352,8 @@ $ENV{MSBFLAGS}="/m";
   <title>Special Considerations for 64-Bit Windows</title>
 
   <para>
-   PostgreSQL will only build for the x64 architecture on 64-bit Windows.
+   PostgreSQL will only build for the x64 and ARM64 architectures on 64-bit
+   Windows.
   </para>
 
   <para>

-  prog = '''
 
 int main(void)

 
-	my $targetmachine =

 
 	if ($vsVersion >= '9.00')
 	{
-		push(@pgportfiles, 'pg_crc32c_sse42_choose.c');
-		push(@pgportfiles, 'pg_crc32c_sse42.c');
+		if ($solution->{platform} eq 'ARM64') {
+			push(@pgportfiles, 'pg_crc32c_armv8_choose.c');
+			push(@pgportfiles, 'pg_crc32c_armv8.c');
+		} else {
+			push(@pgportfiles, 'pg_crc32c_sse42_choose.c');
+			push(@pgportfiles, 'pg_crc32c_sse42.c');
+		}
 		push(@pgportfiles, 'pg_crc32c_sb8.c');
 	}
 	else
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index c2acb58df0..f7a5c83c8f 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -67,8 +67,13 @@ sub DeterminePlatform
 		# Examine CL help output to determine if we are in 32 or 64-bit mode.
 		my $output = `cl /help 2>&1`;
 		$? >> 8 == 0 or die "cl command not found";
-		$self->{platform} =
-		  ($output =~ /^\/favor:<.+AMD64/m) ? 'x64' : 'Win32';
+		if ($output =~ /^\/favor:<.+AMD64/m) {
+			$self->{platform} = 'x64';
+		} elsif($output =~ /for ARM64$/m) {
+			$self->{platform} = 'ARM64';
+		} else {
+			$self->{platform} = 'Win32';
+		}
 	}
 	else
 	{
@@ -423,7 +428,8 @@ sub GenerateFiles
 		STDC_HEADERS                        => 1,
 		STRERROR_R_INT                      => undef,
 		USE_ARMV8_CRC32C                    => undef,
-		USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK => undef,
+		USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK =>
+		  ($self->{platform} eq "ARM64") ? 1 : undef,
 		USE_ASSERT_CHECKING => $self->{options}->{asserts} ? 1 : undef,
 		USE_BONJOUR         => undef,
 		USE_BSD_AUTH        => undef,
@@ -438,7 +444,8 @@ sub GenerateFiles
 		USE_PAM                    => undef,
 		USE_SLICING_BY_8_CRC32C    => undef,
 		USE_SSE42_CRC32C           => undef,
-		USE_SSE42_CRC32C_WITH_RUNTIME_CHECK => 1,
+		USE_SSE42_CRC32C_WITH_RUNTIME_CHECK =>
+		  ($self->{platform} eq "x64" || $self->{platform} eq "Win32") ? 1 : undef,
 		USE_SYSTEMD                         => undef,
 		USE_SYSV_SEMAPHORES                 => undef,
 		USE_SYSV_SHARED_MEMORY              => undef,
@@ -1013,7 +1020,7 @@ sub AddProject
 			$proj->AddLibrary(
 				$self->{options}->{gss} . '\lib\i386\gssapi32.lib');
 		}
-		else
+		elsif ($self->{platform} eq 'x64')
 		{
 			$proj->AddLibrary(
 				$self->{options}->{gss} . '\lib\amd64\krb5_64.lib');
@@ -1022,6 +1029,10 @@ sub AddProject
 			$proj->AddLibrary(
 				$self->{options}->{gss} . '\lib\amd64\gssapi64.lib');
 		}
+		else
+		{
+			die "Kerberos not supported with ARM64\n";
+		}
 	}
 	if ($self->{options}->{iconv})
 	{
@@ -1037,12 +1048,19 @@ sub AddProject
 			$proj->AddLibrary($self->{options}->{icu} . '\lib\icuuc.lib');
 			$proj->AddLibrary($self->{options}->{icu} . '\lib\icudt.lib');
 		}
-		else
+		elsif ($self->{platform} eq 'x64')
 		{
 			$proj->AddLibrary($self->{options}->{icu} . '\lib64\icuin.lib');
 			$proj->AddLibrary($self->{options}->{icu} . '\lib64\icuuc.lib');
 			$proj->AddLibrary($self->{options}->{icu} . '\lib64\icudt.lib');
 		}
+		else
+		{
+			# aarch64.
+			$proj->AddLibrary($self->{options}->{icu} . '\libARM64\icuin.lib');
+			$proj->AddLibrary($self->{options}->{icu} . '\libARM64\icuuc.lib');
+			$proj->AddLibrary($self->{options}->{icu} . '\libARM64\icudt.lib');
+		}
 	}
 	if ($self->{options}->{xml})
 	{
diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index d6bed1ce15..4882d37faf 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -120,9 +120,9 @@ sub writedef
 	{
 		my $isdata = $def->{$f} eq 'data';
 
-		# Strip the leading underscore for win32, but not x64
+		# Strip the leading underscore for win32, but not x64 and aarch64
 		$f =~ s/^_//
-		  unless ($arch eq "x86_64");
+		  unless ($arch eq "x86_64" || $arch eq "aarch64");
 
+		  . "    arch: x86 | x86_64 | aarch64\n"
 
 usage("arch: $arch")
 
 my @files;
 
-- 
2.38.1

---

From: Niyas Sait <[email-masked]>
Date: Mon, 5 Dec 2022 08:54:52 +0000

> lib/VC/sslcrypto32MD.lib, but for arm64.

I tried that installer. And I can see following libraries installed in 
lib/VC location.

libcryptoarm64MD.lib
libcryptoarm64MDd.lib
libcryptoarm64MT.lib
libcryptoarm64MTd.lib
libsslarm64MD.lib
libsslarm64MDd.lib
libsslarm64MT.lib
libsslarm64MTd.lib

> per se the double colon after the question mark.

Yes I did a full build. I am not sure why I didn't see any error with 
that. My perl skills are very limited and I started with something bit 
more naive like "$self->{platform} == "ARM64" ? : 1 : undef" But that 
didn't work and I changed to using eq and the compilation was fine. 
Thanks for fixing the patch.

 > For now, please find attached an updated patch with all the fixes I

Thanks. I did a quick sanity build with your updated patch and looks fine.

-- 
Niyas

---

From: Andres Freund <[email-masked]>
Date: Mon, 5 Dec 2022 10:14:49 -0800

Hi,

> tree at any time in its history.

I'd actually argue that we should just not add win32 support to
src/tools/msvc/.

>  static __forceinline void

This looks somewhat wrong to me. We end up with some ifdefs on the function
defintion level, and some others inside the function body. I think it should
be either or.

> +    have_optimized_crc = true

Why does this need to be hardcoded? The compiler probe should just work for
msvc.

> +      have_optimized_crc = true

And then this reindent wouldn't be needed.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Tue, 6 Dec 2022 08:31:16 +0900

> I'd actually argue that we should just not add win32 support to

Are you arguing about ripping out the existing win32 or do nothing for
arm64?  I guess the later, but these words also sound like the former
to me.
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Mon, 5 Dec 2022 15:48:45 -0800

Hi,

> arm64?  I guess the later, but these words also sound like the former

Yes, that's a typo. I meant that we shouldn't add arm64 support to
src/tools/msvc/.

I do think we should rip out src/tools/msvc/ soon-ish, but we need
buildfarm support first.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Tue, 6 Dec 2022 09:16:17 +0900

> Yes, that's a typo. I meant that we shouldn't add arm64 support to

Okay, thanks.

> I do think we should rip out src/tools/msvc/ soon-ish, but we need

Sure, let's see where it goes.
--
Michael

---

From: Niyas Sait <[email-masked]>
Date: Mon, 12 Dec 2022 13:38:37 +0000

Hi,

> I'd actually argue that we should just not add win32 support to

I think the old build system specific part is really minimal in the 
patch. I can strip out those if that's preferred.

> defintion level, and some others inside the function body. I think it should

Ok, I can add an MSVC/ARM64 specific function.

> Why does this need to be hardcoded? The compiler probe should just work for

There are couple of minor issues in the code probe with MSVC such as 
arm_acle.h needs to be removed and requires an explicit import of 
intrin.h. But even with those fixes, USE_ARMV8_CRC32C would be set and 
no runtime CRC extension check will be performed. Since CRC extension is 
optional in ARMv8, It would be better to use the CRC variant with 
runtime check. So I end up following the x64 variant and hardcoded the 
flags in case of ARM64 and MSVC.

-- 
Niyas

---

From: Michael Paquier <[email-masked]>
Date: Tue, 13 Dec 2022 08:56:33 +0900

On Mon, Dec 12, 2022 at 01:38:37PM +0000, Niyas Sait wrote:
> I think the old build system specific part is really minimal in the patch. I
> can strip out those if that's preferred.

Removing all the changes from src/tools/msvc/ is an approach that
works for me.

> arm_acle.h needs to be removed and requires an explicit import of intrin.h.
> But even with those fixes, USE_ARMV8_CRC32C would be set and no runtime CRC
> extension check will be performed. Since CRC extension is optional in ARMv8,
> It would be better to use the CRC variant with runtime check. So I end up
> following the x64 variant and hardcoded the flags in case of ARM64 and MSVC.

Hm..  Andres, do you have access to Windows hosts with ARM64
underneath to validate any of that?  No way to blame Cirrus for not
having this option, they already do a lot :)
--
Michael

---

From: Niyas Sait <[email-masked]>
Date: Fri, 16 Dec 2022 10:52:23 +0000

> Removing all the changes from src/tools/msvc/ is an approach that

I've attached a new version (v7) that removes changes to support old 
MSVC build system.

-- 
Niyas

From 50e8affb5bcb3f6a50a223053fc92145a5709e82 Mon Sep 17 00:00:00 2001
From: Niyas Sait <[email-masked]>
Date: Fri, 16 Dec 2022 10:45:56 +0000
Subject: [PATCH v7] Enable postgres native build for windows-arm64 platform

- Add support for meson build
- Add arm64 definition of spin_delay function
- Exclude arm_acle.h import for MSVC
---
 doc/src/sgml/install-windows.sgml |  3 ++-
 meson.build                       | 33 +++++++++++++++++++------------
 src/include/storage/s_lock.h      | 20 +++++++++++++++++--
 src/port/pg_crc32c_armv8.c        |  2 ++
 src/tools/msvc/gendef.pl          |  8 ++++----
 5 files changed, 46 insertions(+), 20 deletions(-)

   <para>
 
   <para>

-  prog = '''
 
 int main(void)

diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 8b19ab160f..94d2380393 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -707,15 +707,31 @@ typedef LONG slock_t;

-/* If using Visual C++ on Win64, inline assembly is unavailable.
- * Use a _mm_pause intrinsic instead of rep nop.
+/*
+ * If using Visual C++ on Win64, inline assembly is unavailable.
+ * Use architecture specific intrinsics.
  */
 #if defined(_WIN64)
+/*
+ * For Arm64, use __isb intrinsic. See aarch64 inline assembly definition for details.
+ */
+#ifdef _M_ARM64
+static __forceinline void
+spin_delay(void)
+{
+	 /* Reference: https://learn.microsoft.com/en-us/cpp/intrinsics/arm64-intrinsics#BarrierRestrictions */
+	__isb(_ARM64_BARRIER_SY);
+}
+#else
+/*
+ * For x64, use _mm_pause intrinsic instead of rep nop.
+ */
 static __forceinline void
 spin_delay(void)
 {
 	_mm_pause();
 }
+#endif
 #else
 static __forceinline void
 spin_delay(void)
diff --git a/src/port/pg_crc32c_armv8.c b/src/port/pg_crc32c_armv8.c
index 9e301f96f6..981718752f 100644
--- a/src/port/pg_crc32c_armv8.c
+++ b/src/port/pg_crc32c_armv8.c
@@ -14,7 +14,9 @@
  */
 #include "c.h"
 
+#ifndef _MSC_VER
 #include <arm_acle.h>
+#endif

 

 usage("arch: $arch")
 
 my @files;
 
-- 
2.38.1.windows.1