Feedback-ID: id4a34324:Fastmail
Date: Sat, 5 Nov 2022 11:31:36 -0700
From: Andres Freund <andres@anarazel.de>
To: Niyas Sait <niyas.sait@linaro.org>
Cc: Ian Lawrence Barwick <barwick@gmail.com>,
	Michael Paquier <michael@paquier.xyz>, Tom Lane <tgl@sss.pgh.pa.us>,
	Thomas Munro <thomas.munro@gmail.com>,
	Julien Rouhaud <rjuju123@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Add native windows on arm64 support
Message-ID: <20221105183136.hfxyl5dclxdcoyih@awork3.anarazel.de>
References: <Yw6e4dA4mmusdEsx@paquier.xyz>
 <Yw6oBJgXWnJsy3SE@paquier.xyz>
 <3886822.1661905738@sss.pgh.pa.us>
 <Yw77DNnYl1qkaW1S@paquier.xyz>
 <20476.1661967233@sss.pgh.pa.us>
 <Yw/9pgS35urJZu/7@paquier.xyz>
 <271611.1661998926@sss.pgh.pa.us>
 <YxA4F7EB3SBenvva@paquier.xyz>
 <CAB8KJ=jEZZa=2sgtpjq4raTaqh3vxx5GFFKWF=OZFHo6vp0cGg@mail.gmail.com>
 <CAFPTBD8CCT9BNdQQ0QELoo7HPqx26_Zkn0jxg9F9u+bPz-jnEg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAFPTBD8CCT9BNdQQ0QELoo7HPqx26_Zkn0jxg9F9u+bPz-jnEg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20221105183136.hfxyl5dclxdcoyih%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2022-11-03 11:06:46 +0000, Niyas Sait wrote:
> 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.

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
> +	/*
> +	 * 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
>  	_mm_pause();
> +#endif
>  }
>  #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
> +++ b/src/port/pg_crc32c_armv8.c
> @@ -14,7 +14,9 @@
>   */
>  #include "c.h"
>  
> +#ifndef _MSC_VER
>  #include <arm_acle.h>
> +#endif
>  
>  #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