Smalltalk › Squeak › Squeak VM

[OpenSmalltalk/opensmalltalk-vm] Fix #484 [Win64]Cannot save and load image files with a heap whose size is more than 0xff ff ff ff (~4.1GB) (#485)

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

9 messages Options

David T Lewis

[OpenSmalltalk/opensmalltalk-vm] Fix #484 [Win64]Cannot save and load image files with a heap whose size is more than 0xff ff ff ff (~4.1GB) (#485)

The problem is actually when there is only one segment of memory of more of 0xff ff ff ff to read or to write to disk.

This fix the issue and it is tested with a real case on windows.

I added some checks on function return.

Let me know if changes are to be done to follows some conventions or optimizations.

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

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

Commit Summary

Fix Issue when opening heaps of more than MAX DWORD size
Add function return test

File Changes

M platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c (48)

Patch Links:

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

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Fix #484 [Win64]Cannot save and load image files with a heap whose size is more than 0xff ff ff ff (~4.1GB) (#485)

@j4yk commented on this pull request.

In platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c:

> @@ -37,6 +37,7 @@ extern struct VirtualMachine *interpreterProxy;

 

 #define true  1

 #define false 0

+static const DWORD MAX_DWORD = 4294967295;

⬇️ Suggested change

-static const DWORD MAX_DWORD = 4294967295;

+static const DWORD MAX_DWORD = MAXDWORD;

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

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Fix #484 [Win64]Cannot save and load image files with a heap whose size is more than 0xff ff ff ff (~4.1GB) (#485)

In reply to this post by David T Lewis

@j4yk commented on this pull request.

In platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c:

> @@ -37,6 +37,7 @@ extern struct VirtualMachine *interpreterProxy;
 
 #define true  1
 #define false 0
+static const DWORD MAX_DWORD = 4294967295;

...so really one could just use the constant provided by the Windows headers below.

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

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Fix #484 [Win64]Cannot save and load image files with a heap whose size is more than 0xff ff ff ff (~4.1GB) (#485)

In reply to this post by David T Lewis

@j4yk commented on this pull request.

In platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c:

>    position = sqImageFilePosition(h);
-  ReadFile((HANDLE)(h-1), (LPVOID) ptr, count*sz, &dwReallyRead, NULL);
-  while(dwReallyRead != (DWORD)(count*sz)) {
-    DWORD err = GetLastError();
-    if(sqMessageBox(MB_ABORTRETRYIGNORE, TEXT("Squeak Warning"),TEXT("Image file read problem (%d out of %d bytes read)"), dwReallyRead, count*sz)
-       == IDABORT) return (dwReallyRead / sz);
-    sqImageFileSeek(h, position);
-    ReadFile((HANDLE)(h-1), (LPVOID) ptr, count*sz, &dwReallyRead, NULL);
+  while (reallyRead != totalToRead) {
+    DWORD toRead = (totalToRead - reallyRead) > (size_t)MAX_DWORD ? MAX_DWORD : totalToRead - reallyRead;
+    BOOL ret = ReadFile((HANDLE)(h - 1), (LPVOID)((sqInt)ptr + (sqInt)reallyRead), toRead, &dwReallyRead, NULL);
+    reallyRead += dwReallyRead;
+
+    if (!ret | dwReallyRead != toRead) {

Not sure whether this is a bug or "clever" code to use the bitwise or. I suggest using the boolean operators (&& and ||) instead for clarity.

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

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Fix #484 [Win64]Cannot save and load image files with a heap whose size is more than 0xff ff ff ff (~4.1GB) (#485)

In reply to this post by David T Lewis

@j4yk commented on this pull request.

In platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c:

>    DWORD dwReallyWritten;
-  WriteFile((HANDLE)(h-1), (LPVOID) ptr, count*sz, &dwReallyWritten, NULL);
-  return (size_t) (dwReallyWritten / sz);
+  size_t totalToWrite = count * sz;
+  squeakFileOffsetType position;
+
+  position = sqImageFilePosition(h);
+  while (reallyWritten != totalToWrite) {
+    DWORD toWrite = (totalToWrite - reallyWritten) > (size_t) MAX_DWORD ? MAX_DWORD : totalToWrite - reallyWritten;
+    BOOL ret = WriteFile((HANDLE)(h - 1), (LPVOID)((sqInt)ptr + (sqInt) reallyWritten), toWrite, &dwReallyWritten, NULL);
+    reallyWritten += dwReallyWritten;
+
+    if (!ret | dwReallyWritten != toWrite) {

See above.

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

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Fix #484 [Win64]Cannot save and load image files with a heap whose size is more than 0xff ff ff ff (~4.1GB) (#485)

In reply to this post by David T Lewis

@eliotmiranda approved this pull request.

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

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Fix #484 [Win64]Cannot save and load image files with a heap whose size is more than 0xff ff ff ff (~4.1GB) (#485)

In reply to this post by David T Lewis

Merged #485 into Cog.

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

fniephaus

Re: [OpenSmalltalk/opensmalltalk-vm] Fix #484 [Win64]Cannot save and load image files with a heap whose size is more than 0xff ff ff ff (~4.1GB) (#485)

In reply to this post by David T Lewis

@Jakob: Eliot seems to have merged this without incorporating your feedback. Could you please open another PR with your suggestions?

Thanks,

Fabio

On Sat, 11 Apr 2020 at 9:46 pm, Jakob Reschke <[hidden email]> wrote:

@j4yk commented on this pull request.

In platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c:

>    DWORD dwReallyWritten;
-  WriteFile((HANDLE)(h-1), (LPVOID) ptr, count*sz, &dwReallyWritten, NULL);
-  return (size_t) (dwReallyWritten / sz);
+  size_t totalToWrite = count * sz;
+  squeakFileOffsetType position;
+
+  position = sqImageFilePosition(h);
+  while (reallyWritten != totalToWrite) {
+    DWORD toWrite = (totalToWrite - reallyWritten) > (size_t) MAX_DWORD ? MAX_DWORD : totalToWrite - reallyWritten;
+    BOOL ret = WriteFile((HANDLE)(h - 1), (LPVOID)((sqInt)ptr + (sqInt) reallyWritten), toWrite, &dwReallyWritten, NULL);
+    reallyWritten += dwReallyWritten;
+
+    if (!ret | dwReallyWritten != toWrite) {

See above.

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

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Fix #484 [Win64]Cannot save and load image files with a heap whose size is more than 0xff ff ff ff (~4.1GB) (#485)

In reply to this post by David T Lewis

@VincentBlondeau commented on this pull request.

In platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c:

>    position = sqImageFilePosition(h);
-  ReadFile((HANDLE)(h-1), (LPVOID) ptr, count*sz, &dwReallyRead, NULL);
-  while(dwReallyRead != (DWORD)(count*sz)) {
-    DWORD err = GetLastError();
-    if(sqMessageBox(MB_ABORTRETRYIGNORE, TEXT("Squeak Warning"),TEXT("Image file read problem (%d out of %d bytes read)"), dwReallyRead, count*sz)
-       == IDABORT) return (dwReallyRead / sz);
-    sqImageFileSeek(h, position);
-    ReadFile((HANDLE)(h-1), (LPVOID) ptr, count*sz, &dwReallyRead, NULL);
+  while (reallyRead != totalToRead) {
+    DWORD toRead = (totalToRead - reallyRead) > (size_t)MAX_DWORD ? MAX_DWORD : totalToRead - reallyRead;
+    BOOL ret = ReadFile((HANDLE)(h - 1), (LPVOID)((sqInt)ptr + (sqInt)reallyRead), toRead, &dwReallyRead, NULL);
+    reallyRead += dwReallyRead;
+
+    if (!ret | dwReallyRead != toRead) {

It is not clever code here. Thanks for spotting it!

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.