[PATCH] Add native windows on arm64 support

On Wed, Feb 23, 2022 at 11:09 PM Niyas Sait <niyas.sait@linaro.org> wrote:

I have created a patch that adds support for building Postgres for the windows/arm64 platform using the MSVC toolchain. Following changes have been included

1. Extend MSVC scripts to handle ARM64 platform.
2. Add arm64 definition of spin_delay function.
3. Exclude arm_acle.h import with MSVC compiler.

Compilation steps are consistent and similar to other windows platforms.

The change has been tested on windows/x86_64 and windows/arm64 and all regression tests passes on both platforms.

+    # arm64 linker only supports dynamic base address
+    my $cfgrandbaseaddress = $self->{platform} eq 'ARM64' ? 'true' : 'false';
...
+      <RandomizedBaseAddress>$cfgrandbaseaddress</RandomizedBaseAddress>

Does that mean that you can't turn off ASLR on this arch? Does it
cause random backend failures due to inability to map shared memory at
the expected address? Our experience with EXEC_BACKEND on various
Unix systems tells us that you have to turn off ASLR if you want 100%
success rate on starting backends, but I guess it depends on the
details of how the randomisation is done.

Any interest in providing a build farm animal, so that we could know
that this works?

Thanks, Thomas for reviewing the patch!

Yes, we cannot turn off ASLR for Arm64 targets with MSVC. I haven't seen
any issues so far on win/arm64 with this option turned off. I guess it
should be okay. If we really need ASLR turned off, then I guess might have
to use clang.

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

Niyas

On Fri, 18 Mar 2022 at 20:50, Thomas Munro <thomas.munro@gmail.com> wrote:

Hi,

Please don't top-post here. See
https://wiki.postgresql.org/wiki/Mailing_Lists#Email_etiquette_mechanics.

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.

On Tue, Mar 22, 2022 at 11:30 PM Julien Rouhaud <rjuju123@gmail.com> 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...

Hi,

On 2022-03-23 16:30:58 +1300, Thomas Munro wrote:

On Tue, Mar 22, 2022 at 11:30 PM Julien Rouhaud <rjuju123@gmail.com> 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...

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

On Thu, Mar 24, 2022 at 2:15 PM Andres Freund <andres@anarazel.de> wrote:

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.

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.

On Fri, Mar 25, 2022 at 11:38:44AM +1300, Thomas Munro wrote:

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.

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

Michael Paquier <michael@paquier.xyz> writes:

On Fri, Mar 25, 2022 at 11:38:44AM +1300, Thomas Munro wrote:

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.

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.

I dunno, the lack of any in-house capability for this makes me very
nervous. If it causes problems down the road, how will we debug it?
So it seems like the sort of patch to put in at the beginning of a
development cycle, not post-feature-freeze.

regards, tom lane

On 7 Apr 2022, at 15:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael@paquier.xyz> writes:

On Fri, Mar 25, 2022 at 11:38:44AM +1300, Thomas Munro wrote:

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.

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.

I dunno, the lack of any in-house capability for this makes me very
nervous. If it causes problems down the road, how will we debug it?

If those with an interest in such platform support cannot spare the cycles to
at least run a buildfarm member, then it seems a stretch for us to maintain
such support with any confidence.

So it seems like the sort of patch to put in at the beginning of a
development cycle, not post-feature-freeze.

+1

--
Daniel Gustafsson https://vmware.com/

Hi,

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.

We've traditionally been somewhat relaxed about adding support for new
platforms, on similar notions. That said:

So it seems like the sort of patch to put in at the beginning of a
development cycle, not post-feature-freeze.

There doesn't seem to be a great urgency, and there's plenty stuff going on
right now. I can see us merging it post branching off, and then backpatching
it a bit later?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> 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

Hi,

On 2022-04-07 13:42:49 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> 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.

OP said that they might be able to:
/messages/by-id/CAFPTBD_NZb=q_6uE-QV+S+pm=Rc9sBKrg8CQ_Y3dc27Awrm7cQ@mail.gmail.com

Niyas, any updates on that?

Greetings,

Andres Freund

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]https://github.com/nsait-linaro/postgres.git
which contains the proposed patch on a Windows Arm64 machine.

I made a request to add a new machine to build farm through [2]https://buildfarm.postgresql.org/cgi-bin/register-form.pl.

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

On Thu, 7 Apr 2022 at 18:54, Andres Freund <andres@anarazel.de> wrote:

On Tue, Apr 19, 2022 at 03:22:30PM +0100, Niyas Sait wrote:

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

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
change has been merged.

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

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.

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

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

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

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?

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

+   # 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.

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

Niyas

On Wed, Apr 20, 2022 at 10:43:06AM +0100, Niyas Sait wrote:

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.

MSVC linker doesn't allow non-random base addresses for Arm64 platforms.
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

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.

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]https://developercommunity.visualstudio.com/t/LINK-:-fatal-error-LNK1246:-DYNAMICBAS/10020163 on the
community channel of Visual Studio.

Niyas

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

On Thu, 21 Apr 2022 at 05:07, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Apr 21, 2022 at 10:21:04AM +0100, Niyas Sait wrote:

The following error occurs:

LINK : fatal error LNK1246: '/DYNAMICBASE:NO' not compatible with 'ARM64'
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.

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.

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

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

Microsoft updated documentation [1]https://docs.microsoft.com/en-us/cpp/build/reference/dynamicbase?view=msvc-170 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
https://docs.microsoft.com/en-us/cpp/build/reference/dynamicbase?view=msvc-170

On Mon, 25 Apr 2022 at 02:17, Michael Paquier <michael@paquier.xyz> wrote: