aarch64: Allow FFI_WIN64 for winelib

[AndreRH]

No description provided.

[atgreen]

Thanks for this. Please fix indentation (2, not 4). Also, is there any way to test this with our CI infrastructure?

[AndreRH]

Thx for the reply!
Indentation is now 2 and I rebased to current master
It's technically possible to integrate it into the CI, but hard to do as you need a mingw toolchain for aarch64
Note that my change is already used in Hangover:
https://github.com/AndreRH/hangover/blob/master/dlls/include/va_helper_impl.h

[atgreen]

Thanks. I've merged this. libffi/.travis/install.sh is where we download special build tools for CI testing. Where can we get a mingw toolchain for aarch64?

[AndreRH]

Thx!
I'd say here: https://github.com/mstorsjo/llvm-mingw

[tresf]

Where can we get a mingw toolchain for aarch64?

wget -qO- https://github.com/mstorsjo/llvm-mingw/releases/download/20201020/llvm-mingw-20201020-msvcrt-ubuntu-18.04.tar.xz | tar xJvf - --strip 1 -C ./llvm-mingw
export PATH="$(pwd)/llvm-mingw/bin:$PATH"

CI testing

I assume you mean testing the compile portion. AFAIK the actual aarch64 unit tests require QEMU or native hardware.

[tresf]

@AndreRH, hi, I'm working with a downstream project to get Windows aarch64 support which works without this patch, but severely breaks with this patch. Do you (or any other contributors) have any insight as to why this patch would cause Windows ARM64 binaries to stop working properly? For now we've backed this change out downstream.

[AndreRH]

Could you check that the ABI in use is FFI_WIN64? it should be the same as FFI_DEFAULT_ABI on win32

[tresf]

Could you check that the ABI in use is FFI_WIN64? it should be the same as FFI_DEFAULT_ABI on win32

Hi, I'm not certain how to do this. I work with the library through a 3rd-party product which leverages the msvcc.sh to target Windows aarch64. When I cherry-pick this commit, all many of the 3rd-party unit tests start to fail.

I'd be happy to fire some of the upstream unit tests -- assuming they're written and working for Windows aarch64 -- but I'd have to get aquatinted with the build system.

I'm also not afraid to use a cross-compiler to build, but I ultimately need to use the Windows aarch64 machine for testing.

[matthiasblaesing]

@tresf you can output the ABI that is choosen at runtime by modifying dispatch.c. For the normal calls (non-direct) the call flows through dispatch. Add:

  printf("Using ABI: %d\n", abi);

at line 645 (that is after the calling convention is determined).

To give some context for others: The problem is observed when testing JNA on aarch64.

[tresf]

  printf("Using ABI: %d\n", abi);

Without this patch (and passing unit tests) it shows Using ABI: 1.

[AndreRH]

And with the patch it's supposed to be 2, could you check?

[tresf]

And with the patch it's supposed to be 2, could you check?

Correct, with the patch (and failing unit tests) it shows Using ABI: 2.

[AndreRH]

The idea was to run the same code with FFI_WIN64 as the ifdef _WIN32 did before. That way people can select FFI_WIN64 on non-windows platforms.
As it clearly is selected as expected, I have no clue right now what's going wrong.

[matthiasblaesing]

I looks as if the direct dispatch path in JNA is affected. It seems, that ffi_prep_closure_loc fails: java-native-access/jna#1264 (comment)

[matthiasblaesing]

@tsref tested and indeed ffi_prep_closure_loc fails with FFI_BAD_ABI - the code is pretty clear, it basicly has to explode:

libffi/src/aarch64/ffi.c

Lines 786 to 796 in e70bf98

	ffi_status
	ffi_prep_closure_loc (ffi_closure *closure,
	ffi_cif* cif,
	void (*fun)(ffi_cif*,void*,void**,void*),
	void *user_data,
	void *codeloc)
	{
	if (cif->abi != FFI_SYSV)
	return FFI_BAD_ABI;
	void (*start)(void);

I assume, that the FFI_WIN64 needs to be handled here too.

[AndreRH]

@tresf could you please test this patch? If it works I'll make a new Pull Request:
https://pastebin.com/1QmyCJz1

[tresf]

For some reason I had to apply it by hand, but yes it works, at least with JNA's 617 unit tests. The unit tests are back to what they were before this patch.

[AndreRH]

Thanks for testing!
I guess the manual apply was due to the horrible mix of tabs and spaces.
Anyway, I created a PR: #606