Smalltalk › Squeak › Squeak VM

[OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

16 messages Options

David T Lewis

[OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

This is essentially sanitizing and cleanups of existing win32 platform code.
There's no new feature or behavior change except this one: path passed to security plugin will be interpreted as UTF-8 encoded on windows VM.
These changes are required before cleaning the contract between image and VM, especially about encoding of various strings passed: they should be encoded in UTF8 which seems best for platform interoperability.
It's a step forward for enabling the -DUNICODE as attempted by Tobias about 2 years ago.

It is also very important to reduce the number of compiler warnings. 9 times out of 10, these warnings are false positive, but the tenth is indicating a true problem that we'd better not ignore. If we let the benign warnings overflow us, then we are depriving ourself of a useful tool.

You can view, comment on, or merge this pull request online at:

https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/329

Commit Summary

Protect buffer underflow
Make icon setting 64bits compatible
remove a warning about a control path not returning a value
Use TEXT macro for generic (ASCII or WIDE) string method
Provides a high resolution clock for MSVC
Define some UNIX constants in sqWin32Directory.c, MSVC oblige
Remove a few printf warnings
Prefer DirectX8 in MSVC
Workaround S_ISFIFO to let MSVC compile FilePlugin
Backport Unicode compatibility patches from minheadless variant
Yet another printLastError Unicode compatibility fix in SoundPlugin
Fix another bunch of usage of printLastError uncompatible with Unicode
Fix sqWin32Security.c UNICODE problems
Fix TEXT(VMOPTION('foo"))
Concatenate TEXT constants rather than TEXT concatenated constants
printLastError & warnPrintf take a TCHAR *, not a char *
DPRINTF takes a TCHAR * when it is a warnPrintf in disguise
Splash file and title may use Unicode variant
windowTitle is not a TCHAR *, it is a UTF8 encoded char *
make win23OpenGL UNICODE friendly
Invoke LoadLibraryA when there's no point to internationalize library name
GetProcAddress takes simple byte string procedure name
Use GetLocaleInfoA rather than generic version
remove unused LongFileNameFromPossiblyShortName
the DPRINTF used in sqWin32PluginSupport.c takes a TCHAR *, not a char *

File Changes

M platforms/Cross/plugins/B3DAcceleratorPlugin/sqOpenGLRenderer.c (1)
M platforms/Cross/plugins/FilePlugin/sqFilePluginBasicPrims.c (6)
M platforms/win32/plugins/B3DAcceleratorPlugin/sqWin32OpenGL.c (4)
M platforms/win32/plugins/CroquetPlugin/sqWin32CroquetPlugin.c (2)
M platforms/win32/plugins/HostWindowPlugin/sqWin32HostWindowPlugin.c (4)
M platforms/win32/plugins/LocalePlugin/sqWin32Locale.c (28)
M platforms/win32/plugins/SecurityPlugin/sqWin32Security.c (25)
M platforms/win32/plugins/SoundPlugin/sqWin32Sound.c (72)
M platforms/win32/plugins/SqueakSSL/sqWin32SSL.c (16)
M platforms/win32/vm/sqWin32.h (2)
M platforms/win32/vm/sqWin32Alloc.c (14)
M platforms/win32/vm/sqWin32Backtrace.c (24)
M platforms/win32/vm/sqWin32DirectInput.c (6)
M platforms/win32/vm/sqWin32Directory.c (13)
M platforms/win32/vm/sqWin32ExternalPrims.c (12)
M platforms/win32/vm/sqWin32Heartbeat.c (5)
M platforms/win32/vm/sqWin32Main.c (2)
M platforms/win32/vm/sqWin32PluginSupport.c (48)
M platforms/win32/vm/sqWin32Prefs.c (9)
M platforms/win32/vm/sqWin32Threads.c (4)
M platforms/win32/vm/sqWin32Window.c (120)

Patch Links:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

@krono commented on this pull request.

In platforms/win32/plugins/CroquetPlugin/sqWin32CroquetPlugin.c:

> @@ -8,7 +8,7 @@ static BOOLEAN (__stdcall *RtlGenRandom)(PVOID, ULONG) = NULL;

 int ioGatherEntropy(char *bufPtr, int bufSize) {

   if(!loaded) {

     loaded = 1;

-    hAdvApi32 = LoadLibrary("advapi32.dll");

+    hAdvApi32 = LoadLibraryA("advapi32.dll");

What about

⬇️ Suggested change

-    hAdvApi32 = LoadLibraryA("advapi32.dll");

+    hAdvApi32 = LoadLibrary(TEXT("advapi32.dll"));

instead?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

I really like this PR.

There's one more "fundamental" comment tho:
I'd think that we should not change Windows code to contain any explicit SomFunc…A(…) calls any more.
I'd rather say,

Either do a conversation,
Use a TCHAR variant or,
Explicitly use the SomeFunc…W(…) variant.

PS: I just read the last Windows version not being UNICODE was Windows ME. Everything after that internally is …W in the first place…

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

@krono commented on this pull request.

In platforms/win32/vm/sqWin32.h:

> @@ -263,7 +263,7 @@ extern char imageName[];		/* full path and name to image */
 extern TCHAR imagePath[];		/* full path to image */
 extern TCHAR vmPath[];		    /* full path to interpreter's directory */
 extern TCHAR vmName[];		    /* name of the interpreter's executable */
-extern TCHAR windowTitle[];             /* window title string */
+extern char windowTitle[];             /* window title string */

Why?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

@krono commented on this pull request.

In platforms/win32/vm/sqWin32Backtrace.c:

> @@ -261,14 +261,14 @@ get_modules(void)
 {
 	DWORD moduleCount2, i;
 	HANDLE me = GetCurrentProcess();
-	HANDLE hPsApi = LoadLibrary("psapi.dll");
+	HANDLE hPsApi = LoadLibraryA("psapi.dll");

See above

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

@krono commented on this pull request.

In platforms/win32/vm/sqWin32Heartbeat.c:

> @@ -65,6 +65,11 @@ sqLong ioHighResClock(void) {

 						: "=a" (value)

 						: 

 						: "rdx");

+#elif defined(_MSC_VER)

👍

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

@krono commented on this pull request.

In platforms/win32/vm/sqWin32Main.c:

> @@ -540,7 +540,7 @@ void gatherSystemInfo(void) {
 
 
   {
-    HANDLE hUser = LoadLibrary( "user32.dll" );
+    HANDLE hUser = LoadLibraryA( "user32.dll" );

see above

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

@krono commented on this pull request.

In platforms/win32/vm/sqWin32Prefs.c:

> @@ -207,8 +207,17 @@ void LoadPreferences()
   }
 
   /* get window title from ini file */
+#ifdef UNICODE
+  {
+    TCHAR windowTitleT[MAX_PATH];
+    GetPrivateProfileString(U_GLOBAL, TEXT("WindowTitle"),
+      TEXT(""), windowTitleT, MAX_PATH, squeakIniName);
+    WideCharToMultiByte(CP_UTF8, 0, windowTitleT, -1, windowTitle, MAX_PATH, NULL, NULL);

This needs a check

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

Hi Tobias, about LoadLibraryA, I see nothing in MSDN suggesting that these ASCII versions will be deprecated.
https://docs.microsoft.com/en-us/windows/desktop/api/libloaderapi/nf-libloaderapi-loadlibrarya

The purpose of using W variants is to enable usage of internationalized strings, like in window titles, file names, etc... But when we have a simple explicit ASCII string like "advapi32.dll", using the W variant brings nothing.

About windowTitle, I can explain more: if you look at https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/476f70605a0352dd7528d251f7403e9233716cdb/platforms/win32/vm/sqWin32Window.c

you'll see this: if(*windowTitle) sprintf(titleString, "%s", windowTitle); we first copy the char *windowTitle to the char *titleString. It obviously cannot be UTF16 at this stage, otherwise the copy would be shorten at first ASCII character.

Then titleString is interpreted as UTF8, converted to UTF16 before being displayed as title, line 773:

MultiByteToWideChar(CP_UTF8, 0, titleString, -1, wideTitle, MAX_PATH+20);
SetWindowTextW(stWindow, wideTitle);

This was before I change anything. Also we have this function:

char *ioGetWindowLabel(void) {
  return windowTitle;
}

I think it is for the purpose of communicating the title back to the image. For communicating with the image, we wish using UTF8, so the minimal and simplest change was to just declare windowTitle as char * (which it is now because we can't use -DUNICODE yet), and align the squeak.ini preference to read in UTF16 and convert back to UTF8. You are right, I messed up this part (because I currently compile with -DUNICODE for the purpose of discovering the flaws, when I should rather compile with AND without this option). I will issue another commit and use GetPrivateProfileStringW unconditionnally, which is a lot simpler.

In fact, most of the time, we have to communicate the strings with the image, and for that, we will need to perform UTF8-UTF16 conversions, so compiling with -DUNICODE is not enough and not really a goal per se. Using the generic versions and generic TCHAR *string has only a value if we want to maintain both an ASCII version and a Wide version. But as you said, the ASCII version does not have much interest now, so we could as well directly use W version where we think we need them, or for making the code a bit more uniform (polishing). If it adds un-necessary complexity, I would say just stick to the A version.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

@nicolas-cellier-aka-nice commented on this pull request.

In platforms/win32/vm/sqWin32Prefs.c:

> @@ -207,8 +207,17 @@ void LoadPreferences()
   }
 
   /* get window title from ini file */
+#ifdef UNICODE
+  {
+    TCHAR windowTitleT[MAX_PATH];
+    GetPrivateProfileString(U_GLOBAL, TEXT("WindowTitle"),
+      TEXT(""), windowTitleT, MAX_PATH, squeakIniName);
+    WideCharToMultiByte(CP_UTF8, 0, windowTitleT, -1, windowTitle, MAX_PATH, NULL, NULL);

Definitely! this can only work with -DUNICODE... I will emit a new commit.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

@nicolas-cellier-aka-nice commented on this pull request.

In platforms/win32/vm/sqWin32Prefs.c:

> @@ -207,8 +207,17 @@ void LoadPreferences()
   }
 
   /* get window title from ini file */
+#ifdef UNICODE
+  {
+    TCHAR windowTitleT[MAX_PATH];
+    GetPrivateProfileString(U_GLOBAL, TEXT("WindowTitle"),
+      TEXT(""), windowTitleT, MAX_PATH, squeakIniName);
+    WideCharToMultiByte(CP_UTF8, 0, windowTitleT, -1, windowTitle, MAX_PATH, NULL, NULL);

Wait, I have been abused by the too-short context: this piece of code is precisely protected with a #ifdef UNICODE, so finally I think it is correct.

We may switch to using the W version unconditionnally, but for that, squeakIniName must also be UTF-16 encoded. This is part of larger changes that I have cooking and did not commit yet.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

@krono commented on this pull request.

In platforms/win32/vm/sqWin32Prefs.c:

> @@ -207,8 +207,17 @@ void LoadPreferences()
   }
 
   /* get window title from ini file */
+#ifdef UNICODE
+  {
+    TCHAR windowTitleT[MAX_PATH];
+    GetPrivateProfileString(U_GLOBAL, TEXT("WindowTitle"),
+      TEXT(""), windowTitleT, MAX_PATH, squeakIniName);
+    WideCharToMultiByte(CP_UTF8, 0, windowTitleT, -1, windowTitle, MAX_PATH, NULL, NULL);

hm, I just meant that WideCharToMultiByte needs a success check :)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

I agree with most of what you said.
I'd rather have consistency when "conversing" with the Windows API. I.e. Either TCHAR, Or …A Or …W, but if possible not a mixture, to also in the future understand what we were doing here :)

Apart from that, good work!

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

@nicolas-cellier-aka-nice commented on this pull request.

In platforms/win32/vm/sqWin32Prefs.c:

> @@ -207,8 +207,17 @@ void LoadPreferences()
   }
 
   /* get window title from ini file */
+#ifdef UNICODE
+  {
+    TCHAR windowTitleT[MAX_PATH];
+    GetPrivateProfileString(U_GLOBAL, TEXT("WindowTitle"),
+      TEXT(""), windowTitleT, MAX_PATH, squeakIniName);
+    WideCharToMultiByte(CP_UTF8, 0, windowTitleT, -1, windowTitle, MAX_PATH, NULL, NULL);

OK!

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

@nicolas-cellier-aka-nice pushed 1 commit.

19ebd67 Handle failure to convert WindowTitle preference to UTF8

—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Win64 cleanups (#329)

In reply to this post by David T Lewis

Merged #329 into Cog.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.