Greetings,
The src/plugins/ZipPlugin/ZipPlugin.c function primitiveUpdateGZipCrc32() is currently broken on RiscV64.
This is due to a "boxed" 64 bit int, only 32 bits of which is actually used here.
The choice I made was to add a definition to platforms/Cross/vm/sqMemoryAccess.h
vvv---vvv /* usqInt32 is a 32 bit unsigned integer on 32 and 64 bit systems */ #define usqInt32 unsigned int ^^^---^^^
And then change the local def in primitiveUpdateGZipCrc32()
vvv---vvv usqInt32 crc; ^^^---^^^
[A] This works for arch64/arm64 and riscv64/RV64G in both Cuis and Squeak, but I currently lack other systems to test on.
[B] This _one_ variable use is the _only_ place so far that I have found with this problem. If the VMMaker ZipPlugin generated the unsigned 32 bit int var def directly, sqMemoryAccess.h could remain unchanged.
What is the best way to approach this?
Thanks much, -KenD
Hi Ken,
On Sat, Jul 23, 2022 at 1:44 PM ken.dickey@whidbey.com wrote:
Greetings,
The src/plugins/ZipPlugin/ZipPlugin.c function primitiveUpdateGZipCrc32() is currently broken on RiscV64.
This is due to a "boxed" 64 bit int, only 32 bits of which is actually used here.
The choice I made was to add a definition to platforms/Cross/vm/sqMemoryAccess.h
vvv---vvv /* usqInt32 is a 32 bit unsigned integer on 32 and 64 bit systems */ #define usqInt32 unsigned int ^^^---^^^
And then change the local def in primitiveUpdateGZipCrc32()
vvv---vvv usqInt32 crc; ^^^---^^^
[A] This works for arch64/arm64 and riscv64/RV64G in both Cuis and Squeak, but I currently lack other systems to test on.
[B] This _one_ variable use is the _only_ place so far that I have found with this problem. If the VMMaker ZipPlugin generated the unsigned 32 bit int var def directly, sqMemoryAccess.h could remain unchanged.
What is the best way to approach this?
To fix the plugin. Fixing the generated source isn't fixing the problem; it's just making work. Please stop doing this :-) C plugin source in src/plugins/FooPlugin/FooPlugin.c are generated from the relevant subclasses of InterpreterPlugin in the VMMaker image.
Right now the plugin Smalltalk source in DeflatePlugin>>#primitiveUpdateGZipCrc32 reads:
primitiveUpdateGZipCrc32 "Primitive. Update a 32bit CRC value." <export: true> <primitiveMetadata: #(FastCPrimitiveFlag FastCPrimitiveAlignForFloatsFlag)> "Using AlignForFloats since the arithmetic is potentially vectorizable..." | collection stopIndex startIndex crc length bytePtr | <var: #bytePtr type: #'unsigned char *'> collection := interpreterProxy stackValue: 0. stopIndex := interpreterProxy stackIntegerValue: 1. startIndex := interpreterProxy stackIntegerValue: 2. crc := interpreterProxy positive32BitValueOf: (interpreterProxy stackValue: 3). interpreterProxy failed ifTrue: [^self]. ....
I guess you want to insert a <var: #crc type: #'unsigned int'> alongside the <var: #bytePtr type: #'unsigned char *'> declaration
So you make the edit in a VMMaker image and then submit the version to VMMakerInbox and I integrate it from there. When you're happy doing this we'll promote you to a core VMMaker developer and you can commit directly to VMMaker.
HTH _,,,^..^,,,_ best, Eliot
Hi Ken,
More explanation below.
On Sat, Jul 23, 2022 at 03:00:54PM -0700, Eliot Miranda wrote:
Hi Ken,
On Sat, Jul 23, 2022 at 1:44 PM ken.dickey@whidbey.com wrote:
Greetings,
The src/plugins/ZipPlugin/ZipPlugin.c function primitiveUpdateGZipCrc32() is currently broken on RiscV64.
This is due to a "boxed" 64 bit int, only 32 bits of which is actually used here.
The choice I made was to add a definition to platforms/Cross/vm/sqMemoryAccess.h
vvv---vvv /* usqInt32 is a 32 bit unsigned integer on 32 and 64 bit systems */ #define usqInt32 unsigned int ^^^---^^^
And then change the local def in primitiveUpdateGZipCrc32()
vvv---vvv usqInt32 crc; ^^^---^^^
[A] This works for arch64/arm64 and riscv64/RV64G in both Cuis and Squeak, but I currently lack other systems to test on.
[B] This _one_ variable use is the _only_ place so far that I have found with this problem. If the VMMaker ZipPlugin generated the unsigned 32 bit int var def directly, sqMemoryAccess.h could remain unchanged.
What is the best way to approach this?
To fix the plugin. Fixing the generated source isn't fixing the problem; it's just making work. Please stop doing this :-) C plugin source in src/plugins/FooPlugin/FooPlugin.c are generated from the relevant subclasses of InterpreterPlugin in the VMMaker image.
Right now the plugin Smalltalk source in DeflatePlugin>>#primitiveUpdateGZipCrc32 reads:
primitiveUpdateGZipCrc32 "Primitive. Update a 32bit CRC value." <export: true> <primitiveMetadata: #(FastCPrimitiveFlag FastCPrimitiveAlignForFloatsFlag)> "Using AlignForFloats since the arithmetic is potentially vectorizable..." | collection stopIndex startIndex crc length bytePtr | <var: #bytePtr type: #'unsigned char *'> collection := interpreterProxy stackValue: 0. stopIndex := interpreterProxy stackIntegerValue: 1. startIndex := interpreterProxy stackIntegerValue: 2. crc := interpreterProxy positive32BitValueOf: (interpreterProxy stackValue: 3). interpreterProxy failed ifTrue: [^self]. ....
I guess you want to insert a <var: #crc type: #'unsigned int'> alongside the <var: #bytePtr type: #'unsigned char *'> declaration
So you make the edit in a VMMaker image and then submit the version to VMMakerInbox and I integrate it from there. When you're happy doing this we'll promote you to a core VMMaker developer and you can commit directly to VMMaker.
I am assuming that you are most often using a Cuis image, so some of the things that Eliot is describing may not be familiar. Let me try to put a few things in context for you.
First, please do a "Feature require: 'VMMaker' " in order to load some VM building tools. This is not exactly what is needed to do an update for the plugin in opensmalltalk-vm, but it's close enough to get started (and I'll try to clarify the differences later if there is an interest).
Once loaded, look at DeflatePlugin>>primitiveUpdateGZipCrc32. This is the actual Smalltalk source code for the primitive that you are interested in. When the VM is being "simulated" (which means running slowly but directly using the real Smalltalk source code) then the Smalltalk code is run in your Squeak/Cuis image. When it is compiled into an executable VM program, it is first translated into C code, then compiled as a fast compiled program that runs on Windows or Unix. It is the translation from Smalltalk to C that leads to the type declaration problem that you have identified.
Translation to C is done by CCodeGenerator and its subclasses. The code generator looks at various pragma annotations in the Smalltalk sources, and uses these as hints to do things such as generating C type declarations. If you need to give the code generator a hint to use a specific C type declaration for one of the Smalltalk variables, you can use the var:type: pragma.
In the case of #primitiveUpdateGZipCrc32 the method variable #crc will be translated by default to C type #sqInt, which is a C integer that might be either 32 or 64 bits in size. But for the crc method, you really want this variable to be treated as a 32-bit unsigned integer. Apparently things work by accident on most platforms, but not on RiscV64. Therefore the correct solution is to annotate the Smalltalk method with a pragma that lets the CCodeGenerator know that this variable should be explicitly declared. That is what Eliot is suggesting, and it amounts to nothing more than adding this line to the DeflatePlugin>>primitiveUpdateGZipCrc32 method:
<var: #crc type: #'unsigned int'>
This is a fix that needs to go into both the VMMaker.oscog package (used for the Squeak/Cuis VMs) and also the VMMaker package (classic VM for older Squeak/Cuis images, and also the basis of the VMMaker package in Cuis). These two packages are maintained in Monticello in the source.squeak.org/VMMaker repository, so when Eliot refers to submitting the change to the VMMaker inbox, it means making that one-line change in the VMMaker.oscog package and saving it to the inbox for review and inclusion in the opensmalltalk-vm VM.
If you are interesting in submitting this change, then please do so. We really do need more people who are comfortable making these kinds of updates, and Eliot is trying to encourage you (and others) to get involved in the VM. On the other hand, this might be more than you are interested in taking on just for submitting a small patch, and I would be happy to make the updates on your behalf if you prefer (let me know, it may take me a couple of days to get around to it).
HTH, Dave
Hi Ken,
On Sat, Jul 23, 2022 at 01:44:42PM -0700, ken.dickey@whidbey.com wrote:
Greetings,
The src/plugins/ZipPlugin/ZipPlugin.c function primitiveUpdateGZipCrc32() is currently broken on RiscV64.
When you get a chance, please do a git pull and see if it is working now. I made the one-line patch in VMMaker(s) and generated sources, so hopefully the type declaration issue is resolved.
Dave
vm-dev@lists.squeakfoundation.org