[Vm-dev] CRC fix input

[David T. Lewis]

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 at 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