On Mar 18, 2010, at 11:20 39AM, Igor Stasenko wrote:

On 18 March 2010 12:11, Jean Baptiste Arnaud jbaptiste.arnaud@gmail.com wrote:

...

hi, I have see the immutability bit implementation and i think, it can be easily integrate in the actual VM, the main problem is VMMaker, especially the difference existing between Pharo and Squeak, using VMMaker on Pharo imply to patch it (only small difference, deprecated method, changing method name ), then the main problem is i can change the VM on pharo without creating a incompatible VM for squeak(cause of the patching).

Then i just want to find a solution to have exactly the same version of VMMaker on squeak and pharo. then if someone want to contribute, he can, regardless of this environment .

Then i want to contribute, but i can't.

any solution ?

VMMaker is a standalone project.

-- Best regards, Igor Stasenko AKA sig.

Almost, but not entirely. Certain plugins have their methods implemented in the base image, such as MiscPrimitivePlugin>>primitiveFindSubstring . When the method containing the actual implementation is renamed (as was done in Pharo, and is what Jean Baptiste refers to) the translatedPrimitives mapping in VMMaker no longer works.

The renaming was done to avoid changing all users of findSubstring: in: startingAt: matchTable: , since the primitive returns erroneous results if the substring is a WideString. In Squeak, (well, code is the same in Pharo actually), senders have instead been rewritten to contain guard clauses checking classes of arguments... Let's just hope someone not familiar with the bug decides to use the method for something else, and forgets to add a clause :)

Cheers, Henry

On Mar 18, 2010, at 1:26 10PM, Jean Baptiste Arnaud wrote:

Then what is the solution ? We could have a Pharo immutability bit VM, but that it can be better to share with squeak. (which should be easy, if these differences are fixed ).

I agree.

One solution for the case I mentioned is to not hardcode where the primitives in such cases as MiscPrimitivePlugin are defined, but instead discover them in the image VMMaker is loaded into. Thus you remove the link between VMMaker, and the image it's loaded into defining primitives in certain places, so the same VMMaker can work for both Squeak and Pharo as described.

F.ex. like this:

MiscPrimitivePlugin class>>translatedPrimitives oc := OrderedCollection new. CompiledMethod allInstancesDo: [:each | (each pragmaAt: #primitive:module:) ifNotNil: [:prag | ((prag argumentAt: 2) = 'MiscPrimitivePlugin' and: [each literals includes: #var:declareC:]) ifTrue: [ oc add: (Array with: each methodClass name with: each selector)]]]. ^oc asArray

This assumes the method defining the primitive will include the primitive pragma, which should always be the case. The "each literals includes: #var:declareC:" is a bit of a hack for disambiguating the CompiledMethod defining the primitive from other callers. AFAIK, hints like that are required if the method is to be translated, and you are unlikely to find it in other callers. If anyone has a better idea, I'd appreciate it.

Cheers, Henry

On Mar 19, 2010, at 4:48 27AM, David T. Lewis wrote:

On Thu, Mar 18, 2010 at 02:08:36PM +0100, Henrik Johansen wrote:

...

On Mar 18, 2010, at 1:26 10PM, Jean Baptiste Arnaud wrote:

...

Then what is the solution ? We could have a Pharo immutability bit VM, but that it can be better to share with squeak. (which should be easy, if these differences are fixed ).

I agree.

One solution for the case I mentioned is to not hardcode where the primitives in such cases as MiscPrimitivePlugin are defined, but instead discover them in the image VMMaker is loaded into. Thus you remove the link between VMMaker, and the image it's loaded into defining primitives in certain places, so the same VMMaker can work for both Squeak and Pharo as described.

F.ex. like this:

MiscPrimitivePlugin class>>translatedPrimitives oc := OrderedCollection new. CompiledMethod allInstancesDo: [:each | (each pragmaAt: #primitive:module:) ifNotNil: [:prag | ((prag argumentAt: 2) = 'MiscPrimitivePlugin' and: [each literals includes: #var:declareC:]) ifTrue: [ oc add: (Array with: each methodClass name with: each selector)]]]. ^oc asArray

This assumes the method defining the primitive will include the primitive pragma, which should always be the case. The "each literals includes: #var:declareC:" is a bit of a hack for disambiguating the CompiledMethod defining the primitive from other callers. AFAIK, hints like that are required if the method is to be translated, and you are unlikely to find it in other callers. If anyone has a better idea, I'd appreciate it.

Henry,

Thank you for opening a mantis report on this at http://bugs.squeak.org/view.php?id=7479

I think your pragma idea may work, but IMO this is getting too complicated. Perhaps the change in Pharo was done without realizing the problem it would cause for C code generation.

Well sure, I doubt that was a consideration. However, imo it is the *right* way to fix the issue, by keeping the contract of the public method, but factoring out the act of calling the primitive (which may not fail, but return erroneous results), and implementing the guard in the "old" primitive method. What do you think will happen in Squeak if an external package uses findSubstring: directly? I sincerely doubt they will be aware of its limitations, and have the proper guards in place.

So that means that the primitive is already too clever - it is easy for someone to look at the Smalltalk method and not realize that it is designed to be translated to C. I am afraid that adding pragmas will only make it harder for people to understand how this works.

I suppose you refer to adding pragma usage when resolving locations, since the pragmas themselves are already there.

Using them for resolving was merely the simplest way I could think of which would resolve the primitive definition location for VMMaker, while not hardcoding them.

In sentences describing the functionality of translatedPrimitives, it changes from "These are the hardcoded locations where my methods are defined" to "My methods are defined in places where my primitives are called, and there also exists c-declaration hints". Personally, I don't really see how that is much harder to understand.

So my opinion FWIW is that if we really need different methods in the Squeak and Pharo images, then maybe it is time to reimplement this primitive as a normal primitive, rather than using the magic of MiscPrimitivePlugin translating methods directly from the image.

Sure. That'd probably also be a good change in the sense you don't need to know which image VMMaker was loaded into when the plugin was built to know how it works. How do you redirect images where the MiscPrimitivePlugin method is called, and not the primitive though?

But to be honest I am not convinced that the Pharo method really needs to be renamed to #findSubstringViaPrimitive:in:startingAt:matchTable:. Perhaps there is some other way to accomplish this refactoring without causing compatibility problems?

If you mess with findSubstring:in:startingAt:matchTable: to take care the WideString case is handled correctly, AFAICT you inevitably end up with a solution which would either mean: - It does not end up using the primitive it defines. - It breaks building the primitive with VMMaker.

So the only other option, is to put guard clauses in all senders... Squeak does it. As I noted, in fact Pharo does it as well, but shouldn't, as it already have them centralized in the old findSubstring: method. To me, it does not seem like the right way though, unless your image is static ,and you can ascertain noone else will ever call findSubstring:in:startingAt:matchTable: .

Cheers, Henry

PS. FWIW, as discussed in an earlier thread, the method will probably be renamed again in Pharo, to #primFindSubstring:in:startingAt:matchTable:

On Mar 19, 2010, at 10:53 23AM, Bert Freudenberg wrote:

On 19.03.2010, at 10:44, Henrik Johansen wrote:

...

the method will probably be renamed again in Pharo, to #primFindSubstring:in:startingAt:matchTable:

I see no compelling reason why we shouldn't just apply the same refactoring to Squeak? KISS ;)

• Bert -

It doesn't solve the problem of hardcoded references from Plugins to the core image in the long run, but that'd certainly be a simple solution. :D

The reason I didn't propose it is I didn't want to be seen as forcing Squeak to change just because Pharo changed. (Although, as should be clear from the previous mails I think it's a superior solution to what is in Squeak now)

VMMaker would have to be changed either way to stay compatible :)

Cheers, Henry

On Fri, 19 Mar 2010, Bert Freudenberg wrote:

On 19.03.2010, at 10:44, Henrik Johansen wrote:

...

the method will probably be renamed again in Pharo, to #primFindSubstring:in:startingAt:matchTable:

I see no compelling reason why we shouldn't just apply the same refactoring to Squeak? KISS ;)

What about changing the primitive to fail if the String is "Wide", or make it work with WideStrings?

Levente

• Bert -

On Fri, 19 Mar 2010, Henrik Johansen wrote:

On Mar 19, 2010, at 1:18 17PM, Levente Uzonyi wrote:

...

On Fri, 19 Mar 2010, Bert Freudenberg wrote:

...

On 19.03.2010, at 10:44, Henrik Johansen wrote:

...

the method will probably be renamed again in Pharo, to #primFindSubstring:in:startingAt:matchTable:

I see no compelling reason why we shouldn't just apply the same refactoring to Squeak? KISS ;)

What about changing the primitive to fail if the String is "Wide", or make it work with WideStrings?

Levente

Sure, it's an orthagonal issue though. You'd still need the guards in case an old VM is used, and have to skip those if the image the VM was built with included the primitive code which works correctly. IE. it'd be a real mess, and you'd definately want to have to do it in only one place.

I'd call it parallel, not orthogonal. If the primitive is fixed, then in Squeak 4.2/Pharo 1.2/etc we can remove the checks/hacks in the image and assume that the issue is handled correctly by the vm.

Levente

Cheers, Henry

On 18 March 2010 14:06, Henrik Johansen henrik.s.johansen@veloxit.no wrote:

On Mar 18, 2010, at 11:20 39AM, Igor Stasenko wrote:

...

On 18 March 2010 12:11, Jean Baptiste Arnaud jbaptiste.arnaud@gmail.com wrote:

...

hi, I have see the immutability bit implementation and i think, it can be easily integrate in the actual VM, the main problem is VMMaker, especially the difference existing between Pharo and Squeak, using VMMaker on Pharo imply to patch it (only small difference, deprecated method, changing method name ), then the main problem is i can change the VM on pharo without creating a incompatible VM for squeak(cause of the patching).

Then i just want to find a solution to have exactly the same version of VMMaker on squeak and pharo. then if someone want to contribute, he can, regardless of this environment .

Then i want to contribute, but i can't.

any solution ?

VMMaker is a standalone project.

-- Best regards, Igor Stasenko AKA sig.

Almost, but not entirely. Certain plugins have their methods implemented in the base image, such as MiscPrimitivePlugin>>primitiveFindSubstring . When the method containing the actual implementation is renamed (as was done in Pharo, and is what Jean Baptiste refers to) the translatedPrimitives mapping in VMMaker no longer works.

The renaming was done to avoid changing all users of findSubstring: in: startingAt: matchTable: , since the primitive returns erroneous results if the substring is a WideString. In Squeak, (well, code is the same in Pharo actually), senders have instead been rewritten to contain guard clauses checking classes of arguments... Let's just hope someone not familiar with the bug decides to use the method for something else, and forgets to add a clause :)

Sorry, i missed the whole discussion around that fix, so i can't guess what would be best solution. Or why you had to make VM to behave differently than for Pharo than for Squeak. My only guess, that a proper solution would be to propose fix for VM, then do appropriate fixes in squeak and pharo.

Otherwise, we're going to fork VMs, with all consequences.

Cheers, Henry