[OpenSmalltalk/opensmalltalk-vm] DO NOT INTEGRATE - FOR DISCUSSION ONLY Minimalistic headless x64 msvc2017 (#312)
Ronie submitted a pull request that fixes the build with cygwin for win32 ( https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/311 ) and I'm curious if any of my changes here are still applicable. @ronsaldo could you comment on these... You can view, comment on, or merge this pull request online at:
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/312
-- Commit Summary --
* Fix function call declarations and return types (x64-msvc2017) * Fix unresolved external symbol 'alloca' (x64-msvc2017) * Workaround _controlfp() does not support _MCW_PC or _MCW_IC on x64. Expert review required. * Add architecture define _M_IX86 to ABI (x86-msvc2017) * Workaround hang allocating initial memory (x64-msvc2017). Expert review required. * Better 32bit CMakeLists.txt for SqueakFFIPrims (x86-msvc2017)
-- File Changes --
M CMakeLists.txt (43) M platforms/Cross/plugins/IA32ABI/ia32abicc.c (2) M platforms/Cross/plugins/IA32ABI/x64win64abicc.c (3) M platforms/Cross/plugins/IA32ABI/xabicc.c (2) M platforms/minheadless/common/sqMain.c (1) M platforms/minheadless/windows/sqPlatformSpecific-Win32.c (9) M platforms/minheadless/windows/sqPlatformSpecific-Win32.h (2) M platforms/minheadless/windows/sqWin32Main.c (1) M platforms/minheadless/windows/sqWin32SpurAlloc.c (9)
-- Patch Links --
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/312.patch https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/312.diff
Hi Ben,
On a quick glance, some of these changes are still required, particularly the FPU one, probably the alloca one.
Other changes, such as the #define BUILD_VM_CORE is actually wrong. The minheadless builds the VM itself in a separate library (currently a static library) that can actually be embedded in any application. The public interface for this library is in include/OpenSmalltalkVM.h The problem in windows here was that BUILD_OSVM_STATIC in a previous version was BUILD_SQUEAK_STATIC . After a commentary from @estebanlm I changed the squeak_ prefix for the osvm_ in this public interface.
The public interface file is intended as an stable standalone interface to the vm. So, dependencies on sq.h or similar should not be allowed in this file (In my opinion). The implementation for this interface is in platforms/minheadless/common/sqVirtualMachineInterface.c . This header should be something that an user can copy to /usr/include . osvm_main can be used as an example on how to embed the VM in an application:
OSVM_VM_CORE_PUBLIC OSVMError osvm_main(int argc, const char **argv) { OSVMError error;
/* Global initialization */ error = osvm_initialize(); if(error) return error; /* Parse the command line*/ error = osvm_parseCommandLineArguments(argc, argv); if(error) return error; /* Initialize the VM */ error = osvm_initializeVM(); if(error) return error; /* Load the command line image or the default one. */ error = osvm_loadDefaultImage(); if(error) return error; /* Run Squeak */ error = osvm_run(); /* Shutdown*/ osvm_shutdown(); return error;}
A part of the better 32 bits detection and configuration should also be integrated. When I started the minheadless VM two years ago, there was no stable win64 version of the VM.
Okay. I see my BUILD_VM_CORE was to work around an unknown symbol "_imp_osvm_main", which I guess your BUILD_OSVM_STATIC fixed.
bencoman commented on this pull request.
@@ -2,6 +2,15 @@
cmake_minimum_required(VERSION 3.1) project(OpenSmalltalkVM)
+# The following is temporary while understanding/debugging CMake build. +message("START VARIABLES (A)") +get_cmake_property(_variableNames VARIABLES) +list (SORT _variableNames) +foreach (_variableName ${_variableNames}) + message(STATUS "${_variableName}=${${_variableName}}") +endforeach() +message("END VARIABLES (A)") +
Ignore lines 5-13, they were just adding verbosity for debugging
@@ -77,7 +77,7 @@ static void printCrashDebugInformation(LPEXCEPTION_POINTERS exp);
#define PROCESS_PER_MONITOR_DPI_AWARE 2 #endif
-typedef HRESULT WINAPI (*SetProcessDpiAwarenessFunctionPointer) (int awareness); +typedef HRESULT (WINAPI *SetProcessDpiAwarenessFunctionPointer) (int awareness);
I see this is already covered in #311
@@ -530,6 +540,15 @@ endif()
add_library(${VM_LIBRARY_NAME} ${VM_CORE_LIBRARY_TYPE} ${VM_SOURCES} ${VM_INTERNAL_PLUGIN_SOURCES})
+# The following is temporary while understanding/debugging CMake build. +message("START VARIABLES (B)") +get_cmake_property(_variableNames VARIABLES) +list (SORT _variableNames) +foreach (_variableName ${_variableNames}) + message(STATUS "${_variableName}=${${_variableName}}") +endforeach() +message("END VARIABLES (B)") +
Ignore lines 543-551, they were just adding verbosity for debugging
@@ -28,6 +28,7 @@
*/
#define WIN32_LEAN_AND_MEAN +#define BUILD_VM_CORE
Not required per Ronie's comment. It was to work around an unknown symbol "_imp_osvm_main", which BUILD_OSVM_STATIC fixed already.
@@ -114,7 +114,7 @@ size_t sqImageFileWrite(const void *ptr, size_t sz, size_t count, sqImageFile h)
error "Not Win32!" #endif /* WIN32 */
-int ioSetCursorARGB(sqInt bitsIndex, sqInt w, sqInt h, sqInt x, sqInt y); +sqInt ioSetCursorARGB(sqInt bitsIndex, sqInt w, sqInt h, sqInt x, sqInt y);
Required. Latest Cog branch minheadless/x64 had the same compile error. Including this change only let the build complete.
@@ -115,7 +115,12 @@ void
ioInitPlatformSpecific(void) { /* Setup the FPU */ - _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC | _MCW_PC | _MCW_IC); + /**** 2018.08.07.BenComan TODO, help required. + **** x64 does not support _MCW_PC or _MCW_IC per https://msdn.microsoft.com/en-us/library/e9b52ceh.aspx + ****/ + //Original Line// _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC | _MCW_PC | _MCW_IC); + _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC); +
Required. I've included this per Ronie's comment. Fixes a runtime error.
@@ -25,6 +25,7 @@
* * Author: roniesalg@gmail.com */ +#define BUILD_VM_CORE
Not required per Ronie's comment. It was to work around an unknown symbol "_imp_osvm_main", which BUILD_OSVM_STATIC fixed already.
@@ -5,7 +5,7 @@
* The plugin is misnamed. It should be the AlienPlugin, but its history * dictates otherwise. */ -#if i386|i486|i586|i686 +#if i386|i486|i586|i686|_M_IX86
Not required. Already fixed by PR #311 using... ```#if defined(_M_I386) || defined(_X86_) || defined(i386) || defined(i486) || defined(i586) || defined(i686) || defined(__i386__) || defined(__386__) || defined(X86) || defined(I386)```
@@ -8,7 +8,7 @@
/* null if compiled on other than x86, to get around gnu make bugs or * misunderstandings on our part. */ -#if i386|i486|i586|i686 +#if i386|i486|i586|i686|_M_IX86
Not required. Already fixed by PR #311 using... ```#if defined(_M_I386) || defined(_X86_) || defined(i386) || defined(i486) || defined(i586) || defined(i686) || defined(__i386__) || defined(__386__) || defined(X86) || defined(I386)```
@@ -38,6 +38,9 @@ extern
#endif struct VirtualMachine* interpreterProxy;
+#ifdef _MSC_VER +# define alloca _alloca +#endif
Required. Solves MSVC build error... LNK2019 unresolved external symbol 'alloca' referenced in function callIA32IntegralReturn -- File PharoVMCore.lib(xabicc.c.obj)
Visual Studio 2017" has no "alloca" only "_alloca" https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/alloca
nicolas-cellier-aka-nice commented on this pull request.
@@ -115,7 +115,12 @@ void
ioInitPlatformSpecific(void) { /* Setup the FPU */ - _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC | _MCW_PC | _MCW_IC); + /**** 2018.08.07.BenComan TODO, help required. + **** x64 does not support _MCW_PC or _MCW_IC per https://msdn.microsoft.com/en-us/library/e9b52ceh.aspx + ****/ + //Original Line// _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC | _MCW_PC | _MCW_IC); + _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC); +
I've fixed it in the main OpenSmalltalk VM and will backport. There's a macro FPU_MASK already defined
Hi Ben, I'm currently working on building the Tertf (3D ICC) VM using MSVC Community edition. When I'm done I'll probably migrate the MSVC-specific Makefiles for build.win32* and platforms/win32/plugins into opensmalltalk-vm.
Is this still necessary with the current MSVC stuff?
Whether it's necessary or not, it's useful to have a minimal headless VM. That said, given that the MSVC 2017 build works, the headful makefiles could be used to inform the CMake build (settings, dealing with setjmp/longjmp issues, etc).
Makes sense! Do we want to deal with that herein or a new pr?
If herein is simpler then herein :-)
since 4 out of 6 files have conflicts and some changes are marked "not required" or "already taken care of" by @bencoman, I'd say, starting over is easier?
Cool. Closing...
Closed #312.
vm-dev@lists.squeakfoundation.org
-
Ben Coman -
Eliot Miranda
-
Nicolas Cellier
-
Ronie Salgado
-
Tobias Pape