[foreign] RFR: Windows support

Thu Jan 10 15:19:44 UTC 2019

clang.dll / libclang.so / libclang.dylib is already taken care by native 
library loading mechanism. There should not be any need for System property.

The test failure stack trace on Mac is as follows:

Loading LibClang FFI
Exception in thread "main" java.lang.UnsatisfiedLinkError: no libclang in java.library.path: [/Users/SATHIJEG/bin/llvm/lib]
	at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2700)
	at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:849)
	at java.base/java.lang.Runtime.loadLibrary(Runtime.java:829)
	at java.base/java.lang.System$2.loadLibrary(System.java:2280)
	at java.base/jdk.internal.foreign.LibrariesHelper.loadLibrary(LibrariesHelper.java:169)
	at java.base/java.foreign.Libraries.loadLibrary(Libraries.java:100)
	at jdk.internal.clang/jdk.internal.clang.LibClang.<clinit>(LibClang.java:43)
	at jdk.jextract/com.sun.tools.jextract.parser.Parser.parse(Parser.java:71)
	at jdk.jextract/com.sun.tools.jextract.JextractTool.processHeaders(JextractTool.java:58)
	at jdk.jextract/com.sun.tools.jextract.Main.run(Main.java:264)
	at jdk.jextract/com.sun.tools.jextract.Main.main(Main.java:334)

-Sundar

On 10/01/19, 7:54 PM, Jorn Vernee wrote:
> 2.1) and 2.2) is the srcgen.sh thing. I'm not sure why 
> com/sun/tools/jextract/jclang-ffi/TestJextractFFI.java failed, it 
> looks like your link to the test report got stripped from the email? I 
> changed that test so that it grabs the libclang file name from a 
> system property since it differs on the different platforms ("clang" 
> vs "libclang").
>
> I'll work on an updated webrev for 1) and 2.1) 2.2) in the mean time.
>
> Jorn
>
> Sundararajan Athijegannathan schreef op 2019-01-10 15:18:
>> There are two issues:
>>
>> 1) I had to change make/test/JtregNativeJdk.gmk
>>
>> +    ifeq ($(OPENJDK_TARGET_OS), windows)
>> +        # windows flags
>> +        BUILD_JDK_JTREG_LIBRARIES_CFLAGS_libVector := /arch:AVX
>> +        BUILD_JDK_JTREG_LIBRARIES_CFLAGS_libUpcall := /arch:AVX
>> +        BUILD_JDK_JTREG_LIBRARIES_CFLAGS_libGlobalVariable :=
>> /arch:AVX
>> +    else
>> +        # Enable AVX for vector (Long*) tests
>> +        BUILD_JDK_JTREG_LIBRARIES_CFLAGS_libVector := -mavx
>> +        BUILD_JDK_JTREG_LIBRARIES_CFLAGS_libUpcall := -mavx
>> +        BUILD_JDK_JTREG_LIBRARIES_CFLAGS_libGlobalVariable := -mavx
>> +    endif
>>
>> Because I was getting "clang: error: no such file or directory:
>> '/arch:AVX' on Mac. With the above change, I managed to get passed
>> that failure.
>>
>> 2) Three tests failed.
>>
>> com/sun/tools/jextract/TestDowncall.java [1]:
>> com/sun/tools/jextract/TestUpcall.java [2]:
>> com/sun/tools/jextract/jclang-ffi/TestJextractFFI.java [3]:
>>
>> I think that is srcgen.sh issue you're talking about. If files have to
>> be generated, either we need to automate or pre-generate and be
>> included as part of patch. We cannot expect shell scripts to be
>> manually run before tests (in nightly testing for eg).
>>
>> Please incorporate the above changes and send RFR against the test
>> changes bug. Also please check my change above is fine on Windows.
>>
>> Thanks,
>> -Sundar
>>
>> On 10/01/19, 7:24 PM, Jorn Vernee wrote:
>>
>>>> PS. I'm running the test patch on Mac and Linux to make sure that
>>>> the
>>>> testing is fine on other platforms.
>>>
>>> Thanks.
>>>
>>> One thing I forgot to mention; the generated source files for
>>> UpcallTest and DowncallTest needed changes, so I've removed them and
>>> created the srcgen.sh script in test/jdk/com/sun/tools/jextract to
>>> generate the source files instead. I had this hooked up through
>>> jtreg, but since the native files are compiled before the sources
>>> are generated this didn't generate the files in time, and I never
>>> got around to figuring out how to hook the generation into the build
>>> system.
>>>
>>> So, just be aware that after applying the test-tweaks patch you
>>> still have to run that srcgen.sh script (relies on java 11 on PATH)
>>> to generate the test sources, since the patch deletes them.
>>>
>>> Jorn
>>>
>>> Sundararajan Athijegannathan schreef op 2019-01-10 14:26:
>>> The 4 bugs (including the preexisting one) are as follows:
>>>
>>> 1) Library.getDefault() does not work on Windows
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8211060
>>>
>>> 2) Binder changes are needed to support paname foreign API on
>>> Windows
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8216485
>>>
>>> 3)  jextract LayoutUtils has to be modified for Windows
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8216484
>>>
>>> 4) jextract tests have to be modified to run on Windows platform
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8216483
>>>
>>> PS. I'm running the test patch on Mac and Linux to make sure that
>>> the
>>> testing is fine on other platforms.
>>>
>>> Thanks,
>>> -Sundar
>>>
>>> On 10/01/19, 6:14 PM, Maurizio Cimadamore wrote:
>>> Hi Jorn,
>>> Thanks for the hard work; we are in the process of reviewing the
>>> changes. Here's what we'll do:
>>>
>>> 1) we will create issues for each of the patches (Sundar is doing
>>> that as we speak)
>>> 2) we will try to get easier changes in sooner rather  than later
>>> (that is, patches 1, 3, 4)
>>> 3) in parallel, I'll be doing an in-depth review of your binder
>>> changes and perhaps start a separate thread on that
>>>
>>> Thanks
>>> Maurizio
>>>
>>> On 09/01/2019 21:17, Jorn Vernee wrote:
>>>
>>> Hi,
>>>
>>> I have fixed the last few failing tests on Windows so this is an RFR
>>> for adding Windows support to Panama FFI. There's a lot of changes
>>> so I've split it into 4 patches. We should probably focus on one of
>>> these one at a time, but I wanted to include all the changes needed
>>> to pass the tests at least. It's probably best to start with [2],
>>> since that implements the core functionality.
>>>
>>> Default lib fixes [1] (needed for the default library to work on
>>> Windows)
>>> Hotspot, binder tweaks [2] (Changes to the stub generation and
>>> shared binder code. Includes the Windows implementation of
>>> SystemABI)
>>> jextract tweaks [3] (minor jextract portability changes)
>>> test tweaks [4] (changes needed to run, and pass the tests on
>>> Windows)
>>>
>>> There are some tests that are disabled:
>>> - EmptyStructTest; Empty structs are a GCC extension.
>>> - BadBitfieldTest; this is testing for a problem the MSVC ABI
>>> doesn't seem to have.
>>> - StdioTest; relies on standard system header path, which on
>>> Windows is more tricky to find.
>>> - ComplexTest; Windows complex types work very differently in C
>>> source, so the current test doesn't compile.
>>> - LongDoubleTest; long double is only 64 bits on Windows. This
>>> might need a replacement test still, but it should work the same as
>>> `double` on Windows.
>>> - Getpid; No getpid on Windows in the C standard.
>>> - UnixSystem; Windows is not unix.
>>>
>>> (Some tests specifically for Windows could be added, but I'm leaving
>>> that for a different RFR)
>>>
>>> Changes for [1]:
>>> - replace default library handle with a findEntryInProcess since
>>> Windows doesn't have an RTLD_DEFAULT equivalent.
>>>
>>> Changes for [2]:
>>> - changed upcall/downcall stub generation to follow the MS ABI on
>>> windows (using compiler switches)
>>> - added SKIPs to shuffle recipe generation when e.g. the first
>>> float is passed in the second float register (which can be the case
>>> on windows).
>>> - moved boxValue and unboxValue into SystemABI, since they turned
>>> out to be ABI specific, and made tweaks to the windows version for
>>> handeling structs being passed by value.
>>> - moved upcall stub natives into shared ABI package.
>>> - Let each ABI have it's own VarargsInvoker implementation, since
>>> we need to propagate more info about which arguments are variadic on
>>> windows.
>>> - CallingSequence::storageOffset is no longer relying on
>>> Storage::getStorageIndex, since that is not the same as what's
>>> actually needed; being the index of the binding. (the 2 can differ
>>> on windows, but not on sysv).
>>> - Added Windows ABI implementation classes based on the SysV ones.
>>>
>>> - Let UniversalUpcallHandler write the in-memory-return pointer to
>>> RAX since that is required on Windows (and I think this change is
>>> portable).
>>>
>>> Changes for [3]:
>>> - instead of hardcoding the size of `long` `unsigned long` and
>>> `long double`, delegate to libclang to tell us the size, since these
>>> types have different sizes on the 2 ABIs. (This is pretty ad-hoc, so
>>> probably needs to be improved. But, I have little experience with
>>> the jextract code, so I'm open to suggestion of how to better fix
>>> this)
>>>
>>> Changes for [4]:
>>> - added dll exports for native symbols using compiler switches.
>>> - added test for small (fits-in-register) struct by-value passing,
>>> since windows has special handling for those.
>>> - removed depencies on POSIX functions from some tests where it
>>> wasn't really needed, so they could run on windows.
>>> - propagate clang library name to TestJextractFFI, since on
>>> Windows we need to load "libclang".
>>> - let Runner test use windows specific files to compare with for
>>> bitfields.h and simple.h
>>> - use `long long` instead of `long` in some cases since `long
>>> long` is 64 bits on SysV and MSx64
>>>
>>> Thanks,
>>> Jorn
>>>
>>> [1] :
>>>
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8211060/webrev.01/
>>>
>>> [2] :
>>>
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/windows/webrev.03/
>>>
>>> [3] :
>>>
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/windows_jextract/webrev.00/ 
>>
>>> [4] :
>>>
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/windows_tests/webrev.02/ 
>>
>>
>>
>> Links:
>> ------
>> [1]
>> http://webmail.xs4all.nl/../../../test-support/jtreg_test_jdk_jdk_foreign/com/sun/tools/jextract/TestDowncall.jtr 
>>
>> [2]
>> http://webmail.xs4all.nl/../../../test-support/jtreg_test_jdk_jdk_foreign/com/sun/tools/jextract/TestUpcall.jtr 
>>
>> [3]
>> http://webmail.xs4all.nl/../../../test-support/jtreg_test_jdk_jdk_foreign/com/sun/tools/jextract/jclang-ffi/TestJextractFFI.jtr 
>>