Support for compiling Windows on VS2017 #1697
Comments
|
Do we have an outlook for starting this? |
|
I will check it out to see how to get this work. |
|
After fixing the setting issues with jre-image, paths to MSVC, etc, I am currently working on other issues in config/mk scripts captured during compilation. |
|
Currently it ended up with linking failure on unresolved external symbols as follows: It seems the failure was triggered when linking (VS2017) the .obj files (compiled by mingw) with non-existent functions in libraries in VS2017 (___iob_func was already renamed to acrt_iob_func since VS2015) Here's typical explanations online of what happens to "unresolved external symbol __imp___iob_func" There are a bunch of discussions on "unresolved external symbol __imp___iob_func" but none of them works for our case (tried with legacy_stdio_definitions.lib but didn't work. might be in the wrong way?) I am wondering whether it is possible to get the linker (VS2017) to pick up the static libraries ( __imp___iob_func) supported in mingw. |
|
We can try compiling the BytecodeInterpreter with VS2017 instead of mingw. Perhaps it will work, even though 2010 and 2013 do not. As I recall 2010 and 2013 can't compile BytecodeInterpreter with optimization enabled. It compiles with -O0 but then we have stack related problems at runtime. |
|
THe issue is that by default the new VS uses a bunch of inlined functions for CRT functions, but mingw doesnt pick them up. IIRC this can be fixed by setting the |
|
To verify the suggestion above, I made the following changes: However, it still ended up with the same linking failures as follows: The failures shows -O0 and -DVS12AndHigher were already set up for x86_64-w64-mingw32-g++ and it works in some way but it didn't help to get these obj file linked correctly. My understanding is that mingw does pick up the inlined CRT functions (the error above is kind of different from previous one) but it is unable to replace __imp___iob_func with the renamed acrt_iob_func or it can't find acrt_iob_func in its own lib path during compilation. Double-checked BytecodeInterpreter.obj with dumpbin.exe tool. |
|
Also checked the directories of mingw but acrt_iob_func doesn't exist: |
An aside have we thought of taking advantage now that we're open source to file a bug report with Microsoft [1] on this one so it can be fixed in a future release? [1] https://docs.microsoft.com/en-us/cpp/how-to-report-a-problem-with-the-visual-cpp-toolset |
|
The best case is that mingw guys upgrade their code to keep up with the stdio-related changes in VS2015/2017. However, they didn't do that as they didn't expect we to compile the code in this way. |
I think @charliegracie was in communication at one point, not sure if we got as far as filing a bug. |
|
The situation is quite similar to compiling code with an earlier version of VS (VS2010) and linking them via VS2015/2017, which most users encountered in their projects as discussed on websites. |
|
Currently the build is compiled on Windows 7 in which mingw still sticks with an older version of CRT. The link from Microsoft at https://msdn.microsoft.com/en-us/library/abx4dbyh.aspx says: The CRT Library has been refactored into a two different binaries, a Universal CRT (ucrtbase), which contains most of the standard functionality, and a VC Runtime Library (vcruntime140), which contains the compiler-related functionality, such as exception handling, and intrinsics. The website of mingw-w64 at https://mingw-w64.org/doku.php#mingw-w64 Given that the stuff with "stdio.h" was categorized to UCRT since VS2015, it might be worthy to try installing Cygwin with the latest mingw-w64 packages + VS2017 (plus Windows 10 SDK ? ) on Windows 10 to see whether mingw-w64 compile our code with UCRT. |
|
It turns out the mingw64 package specified in cygwin is still an older version without UCRT support.
|
|
The new version of x86_64-w64-mingw32-g++ works good on in terms of compiling BytecodeInterpreter.cpp, DebugBytecodeInterpreter.cpp and MHInterpreter.cpp on Win 7 after installing the latest version of mingw64 downloaded from https://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win32/Personal%20Builds/mingw-builds/installer/mingw-w64-install.exe So the root cause is that the latest cgywin didn't pick up the new version of mingw64 (which already supports UCRT). The way around this issue is to update our config/setting file (e.g./runtime/makelib/mkconstants.mk.ftl) to point to the new version of mingw64 (which is installed separately/outside of cygwin dir) rather than the outdated one installed in cygwin by default. (Note: the problem has nothing to do with Windows 10) Will keep investigating as there are still other issues during compilation. |
I think he sent then the prepossessed interpreter and they just laughed and said no. |
|
Currently investigating the error related to ddrgen. e.g. for j9ddr_misc29.pdb Comparing the printing messages (from ddrgen) of j9ddr_misc29.pdb created by VS2010 and VS2017 suggests that the existing code in PdbScanner.cpp might have problem in dealing with the case of the nested SymTagUDT (only occurs in VS2017) or something new was generated in j9ddr_misc29.pdb. |
|
Here're the results from j9gc29.pdb and j9thr29.pdb: I didn't find any file containing "_FNV_prime" in the build compiled via VS2010 while there are a bunch of .pdb files containing _FNV_prime (including j9thr29.pdb, j9thr29.pdb) in the build compiled by VS2017. If so, I am wondering whether the new type name needs to be added in a certain config/setting file in ddrgen/pdbscanner or the existing code in setMemberOffset() should address the case. |
According to [1], offset type 10 is An offending field is Perhaps we need to check whether a field is static before worrying about its offset. [1] https://docs.microsoft.com/en-us/visualstudio/debugger/debug-interface-access/locationtype |
|
The problem is it returns LocIsConstant for both "const" and "static const" variables, in which case there is no easy way to tell whether the current field is static or not. |
|
Can you give an example of a non-static field where that happens? |
|
|
Those fields certainly have 'const' type, but they are not compile-time constants so I would not expect offset type |
|
I double-checked the superset.dat generated via VS2010 and VS2017 and it turns out e.g. static const int32_t invalidByteCodeIndex = -1; That means "static" is just ignored when calling get_locationType in the case of "const static" variables. |
|
As for a "const" variable definition, e.g. const uint32_t categoryCode; It means it returns something else (e.g. LocIsThisRel, LocIsBitField, etc) for "const" variables instead of LocIsConstant. If so, the field must be a static if it returns LocIsConstant (Correct me if wrong). Then we can safely change the code to: |
|
With the latest changes above (ibmruntimes/openj9-openjdk-jdk11#3 and #2374), the compilation ended up with a module related exception after copying/rename j9vm_jdk11/jdk11_jvm.lib & .exp to vm/lib: If the merged fixes are correct, then the problem might be related to the module setting at java.base (e.g. src/java.base/share/classes/module-info.java) |
|
The detailed backtrace in java is as follows: Looking at the code of ModuleInfo.doRead() in src/java.base/share/classes/jdk/internal/module/ModuleInfo.java Looking at src/java.base/share/classes/module-info.java (packages exist in there) So it seems the failure to parse package names from java.base/module-info.class triggered the exception. Given that the exposed packages exist in java.base, I need to figure out whether |
If so, it is likely jclse11_29.dll was not generated correctly during compilation or there are still problems with the corresponding config/setting for the library generation. |
|
Investigation shows the problem with jclse11_29 came from the code in J9SigQuitStartup as follows: The problem with Initialization disappear if disabling SigQuit with -Xrs (JIT off) How, the issue with module-info.class still exist with JIT on: So it is hard to determine whether they came from the same root cause or they are totally different issues. |
|
I think the root cause for unresolved external symbol to JVM_BeforeHalt/JVM_AreNestMates is that these natives not exported via j9vm/module.xml. So I made some changes as follows: The compilation ended up with the same result as copying/renaming j9vm_jdk11/jdk11_jvm.lib & .exp to vm/lib, which means copying/renaming j9vm_jdk11/jdk11_jvm.lib was incorrect or at least unnecessary. If so, I need to double-check the generation of jclse11_29.dll as well as all related module.xml to see whether there is anything else should be fixed. |
|
I tried two steps to narrow down the scope:
So the investigation above indicates: |
Given that turning on J9VM_OPT_VALHALLA_NESTMATES enables all code changes of nestMates, I suspect there might be code issues in the existing implementation of nestMates. If so, there is no way to fix them in our script/setting from the compilation perspective (This is the only issue left here). I will double-check all my changes so far in jdk11/OpeJ9/omr and submit them for review soon. FYI: @tajila. |
|
The issues with nestmates should go away once #2459 is merged. |
|
After manually merging the changes at #2459, all problems with nestMates were fixed and the compilation passed without error: Given all issues are fixed in compilation, I will set up to run sanity test to verify its basic functionalities. |
|
A test failure with UnsatisfiedLinkError is detected during the sanity test as follows: Look at the test code: Currently I am running tests with .../build/windows-x86_64-normal-server-release/images/jdk/bin Comparison with the compiled build with openjdk8/open9 shows the j9ben.dll exists in .../build/windows-x86_64-normal-server-release/jdk/bin/compressedrefs but missing in openj9-openjdk-jdk11/build/windows-x86_64-normal-server-release/images/jdk/bin/compressedrefs Looking at the dll files at jdk/bin/compressedrefs and images/jdk/bin/compressedrefs: It seems a lot of test related dll files are missing in images/jdk/bin/compressedrefs except j9ben.dll. @keithc-ca, I suspect the current script in openjdk11 didn't or failed to copy these dll files from /vm to the right place after improving these setting (e.g. OpenJ9.gmk). If so, the issue should be fixed to enable the test execution. |
|
The JDK should have none of those tests DLLs: tests should bring their own DLLs rather than expect the JDK to have them. |
|
We'll have to put the test libraries into images/test I expect. Then PR builds and pipeline builds will have to find them there. |
|
I already finished the sanity test with build/windows-x86_64-normal-server-release/jdk/bin and it shows the compiled build functionally works: |
|
I will prepare all changes I made in compilation for review before addressing the test failure in another issue. |
|
All script/setting changes have been submitted for review: |
|
I don't think this is going to be ready for the 0.10.0 release, deferring. |
|
@pshipton We should be trying to contain this in the 0.10.0 release to match the compiler levels used by OpenJDK. Is there a critical blocker preventing this from making the 0.10.0 release? If so, let's identify it and try to solve it. |
|
I have been working on eclipse-omr/omr#2885 which would be a prerequisite of DDR support with that compiler. |
|
Closing, since Java 11 Windows is now compiling with MSVC 2017. |
OpenJDK is modifying Windows compilation for Java 11+ to use VS2017 and OpenJ9 should support the same.
https://bugs.openjdk.java.net/browse/JDK-8201267
See also #1684
The text was updated successfully, but these errors were encountered: