search

codefrau / SqueakJS

A Squeak Smalltalk VM in Javascript
https://squeak.js.org
MIT License
365 stars 75 forks source link

Problem with failing primitiveStringHash? | Cannot load metacello.sar #115

Closed LinqLover closed 3 years ago

LinqLover commented 3 years ago

I tried to file in this metacello.sar in two images, Squeak5.3-19448-32bit.image and a fresh 64-bit Trunk image. In each case, I get the following error message while the preamble of the SAR file is processed:

MessageNotUnderstood: ByteString>>bitAnd:
ByteString(Object)>>doesNotUnderstand: #bitAnd:
ByteString(String)>>hashWithInitialHash:
ByteString>>hashWithInitialHash:
MCMethodDefinition>>hash
Set>>scanFor:

A closer look at the temporary variables causes confusion:

So, is there a bug in primitiveStringHash that probably misaligns the stack variables in any way? Interesting bug ...

PS: Please let me know if you can reproduce it.

fniephaus commented 3 years ago

No sure what's going on, but the implementation of primitiveStringHash is generated and looks ok: https://github.com/codefrau/SqueakJS/blob/3538c55eb66355a137554aa218e0ffccc3d7575c/plugins/MiscPrimitivePlugin.js#L544-L574

codefrau commented 3 years ago

That primitive looks buggy, because it is called with argCount = 1 so it should pop 2 (string+arg) but actually pops 3.

What does it do in other VMs? Are they using argCount? This was generated many years ago, but I don't think the primitive changed?

codefrau commented 3 years ago

OMG this must have been wrong since 2014. I just tried it with

primitiveStringHash(argCount) { 
   ... 
   interpreterProxy.popthenPush(argCount + 1, hash);
}

and metacello.sar continues installing fine.

However, that means some other generated primitives are likely wrong too 🤔 I have not committed a fix yet.

Thank you for the test case!

codefrau commented 3 years ago

So ca63f69 checks the stack after every primitive. And 4e152a6 fixes an embarrassing number of primitives that left the stack imbalanced or at least did not check argCount. Released as 1.0.1.

krono commented 3 years ago

So ca63f69 checks the stack after every primitive. And 4e152a6 fixes an embarrassing number of primitives that left the stack imbalanced or at least did not check argCount. Released as 1.0.1.

This is always fun :D

LinqLover commented 3 years ago

Great, thanks for fixing! :D

So ca63f69 checks the stack after every primitive.

Out of curiosity, how large is the performance impact of such "assertions in production"? :-)

codefrau commented 3 years ago

@LinqLover please use the vm-dev list for this kind of question/discussion. Nobody’s gonna read through closed tickets ;)