Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

primitiveImageFormatVersion may be not correct #638

Open
marceltaeumel opened this issue May 30, 2022 · 12 comments
Open

primitiveImageFormatVersion may be not correct #638

marceltaeumel opened this issue May 30, 2022 · 12 comments
Labels

Comments

@marceltaeumel
Copy link
Contributor

marceltaeumel commented May 30, 2022

The return value of primitiveImageFormatVersion is hard-coded and does not consider the value of multipleBytecodeSetsActive, which is indeed important when trying to open a particular image with a particular VM.

See:

  • Interpreter >> #imageFormatVersion
  • StackInterpreter >> #imageFormatVersion
  • ObjectMemory >> #imageFormatVersion
  • Spur32BitMemoryManager >> #imageFormatVersion
  • Spur64BitMemoryManager >> #imageFormatVersion

And also:

  • StackInterpreter >> #imageFormatVersionFromSnapshot:
  • StackInterpreter >> #imageFormatVersionForSnapshot

Note that this can be fixed on the image-side by checking primitiveMultipleBytecodeSetsActive. Yet, we have a discrepancy between the definition of "image format" at time of snapshotting and during run-time.

@dtlewis290
Copy link
Contributor

It is a VM bug, sorry for overlooking this. Proposed fix in VMMaker inbox VMMaker.oscog-dtl.3185

@dtlewis290
Copy link
Contributor

Fix is in VMMaker (VMMaker.oscog-dtl.3185), still need to commit generated code and build VM.

@dtlewis290
Copy link
Contributor

@marceltaeumel the fix inVMMaker.oscog-dtl.3185 is trivial, but I need someone else (you or Eliot) to do the code generation this time. The reason is that my generated code has differences that I cannot account for (specifically one value in the primitiveMetadataTable array is generated as 0x100 but should be 0x200, and in several places there are differences in integer type declarations). I have not be able to track down the reason for the differences, so I cannot commit the generated code.

@marceltaeumel
Copy link
Contributor Author

Wow. This is not over. Next issue is in imageSegmentVersion. I will try to fix this on the image side for now. But this is still a bug in the VM primitve 98 primitiveStoreImageSegment... :-(

@marceltaeumel
Copy link
Contributor Author

The biggest issue for an image-side discrimination is that the VMMaker-specific constant 512 ... MultipleBytecodeSetsBitmask is not part of the regular image code. Which is a good thing, I suppose. Still ...

@dtlewis290
Copy link
Contributor

For my understanding - is the concern with imageSegmentVersion a problem for practical reasons (something does not work as expected)? It certainly makes sense that an image segment should have a format number that matches the image from which it was saved, but as a practical matter does anything bad happen if this is not the case?

@OpenSmalltalk-Bot
Copy link

OpenSmalltalk-Bot commented Jun 7, 2022 via email

@dtlewis290
Copy link
Contributor

I am trying to understand the real-world scenario in which this matters. I have an image running Sista bytecodes and my image format is 68533. I save an image segment to disk, and that image segment contains objects that contain compiled code that contain Sista bytecodes. The saved image segment records itself with format number 68021 (not 68533). Later on, someone loads that image segment using a VM that is a year or two old, so the VM does not know about 68533. But the VM probably does know how to run Sista bytecodes because it is only a year or two old. Everything still works, so as a practical matter what is the problem?

@dtlewis290
Copy link
Contributor

Regarding image side code and MultipleBytecodeSetsBitmask in VMMaker, on the image side you would use this (only if necessary, and hopefully it is not necessary):

(ImageFormat fromInteger: 68021) requiresMultipleBytecodeSupport ==> false
(ImageFormat fromInteger: 68533) requiresMultipleBytecodeSupport ==> true

@marceltaeumel
Copy link
Contributor Author

[...] is the concern with imageSegmentVersion a problem for practical reasons [...]
I am trying to understand the real-world scenario in which this matters. [...]

Well, loads of tests fail or err at the moment. Maybe we should just fix the image-side load code for image segments to also accept 68021, even if the imageFormatForSnapshot is 68533.

@marceltaeumel
Copy link
Contributor Author

I committed a quickfix via System-mt.1355. Yet, this topic might need more discussion.

@OpenSmalltalk-Bot
Copy link

OpenSmalltalk-Bot commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants