The Trunk: Monticello-dtl.682.mcz

> David T. Lewis uploaded a new version of Monticello to project The Trunk:
> http://source.squeak.org/trunk/Monticello-dtl.682.mcz
>
> ==================== Summary ====================
>
> Name: Monticello-dtl.682
> Author: dtl
> Time: 13 May 2018, 7:14:27.068827 pm
> UUID: 76fc84e6-7e2b-4a6a-af6b-2d02d11c38eb
> Ancestors: Monticello-nice.681
>
> Sending perform: #== with: anObject to an instance of MCPackageInEnvironment should test identity of the actual instance, not the MCPackage to which it refers. If this is not so, a PluggableDictionary will fail when doing identity checks on its keys, as is the case for a SystemTracer adding all objects to its oop dictionary.
>
> Implement MCPackageInEnvironment>>perform:with: such that performing #== will call primitivePerform, and all other selectors are delegated to the MCPackage as before.
>
> =============== Diff against Monticello-nice.681 ===============
>
> Item was added:
> + ----- Method: MCPackageInEnvironment>>perform:with: (in category 'delegating') -----
> + perform: aSymbol with: anObject
> + "If aSymbol is #== then the sender is trying to perform an identity comparison.
> + This cannot be delegated, because the delegate will be a different object.
> + Implement perform:with: here to protect for this case. An important example
> + is the case of PluggableDictionary, which relies on perform:with: to implement
> + comparisons for an identity dictionary."
> +
> + aSymbol == #==
> + ifTrue: [ ^self privatePerform: aSymbol with: anObject ]
> + ifFalse: [ ^package perform: aSymbol with: anObject "as per doesNotUnderStand:" ]
> + !
>
> Item was added:
> + ----- Method: MCPackageInEnvironment>>privatePerform:with: (in category 'delegating') -----
> + privatePerform: aSymbol with: anObject
> + "aSymbol is #== and the sender is trying to perform an identity comparison.
> + Invoke primitivePerform."
> +
> + <primitive: 83>
> + ^ self primitiveFailed!
>
>

> This update feels a bit hacky to me with its test for a specific selector, so
> better or clearer solutions are welcome. Meanwhile, the prior behavior was
> corrupting identity dictionaries, which seems like a Bad Thing, so I put
> this fix (along with a new unit test) directly into trunk.
>
> The problem showed up as I was trying to get SystemTracer64 working again on
> V3 images, so it was not an easy thing to debug :-/
>
> Dave
>
> On Sun, May 13, 2018 at 11:14:39PM +0000, [hidden email] wrote:
>> David T. Lewis uploaded a new version of Monticello to project The Trunk:
>> http://source.squeak.org/trunk/Monticello-dtl.682.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Monticello-dtl.682
>> Author: dtl
>> Time: 13 May 2018, 7:14:27.068827 pm
>> UUID: 76fc84e6-7e2b-4a6a-af6b-2d02d11c38eb
>> Ancestors: Monticello-nice.681
>>
>> Sending perform: #== with: anObject to an instance of MCPackageInEnvironment should test identity of the actual instance, not the MCPackage to which it refers. If this is not so, a PluggableDictionary will fail when doing identity checks on its keys, as is the case for a SystemTracer adding all objects to its oop dictionary.
>>
>> Implement MCPackageInEnvironment>>perform:with: such that performing #== will call primitivePerform, and all other selectors are delegated to the MCPackage as before.
>>
>> =============== Diff against Monticello-nice.681 ===============
>>
>> Item was added:
>> + ----- Method: MCPackageInEnvironment>>perform:with: (in category 'delegating') -----
>> + perform: aSymbol with: anObject
>> + "If aSymbol is #== then the sender is trying to perform an identity comparison.
>> + This cannot be delegated, because the delegate will be a different object.
>> + Implement perform:with: here to protect for this case. An important example
>> + is the case of PluggableDictionary, which relies on perform:with: to implement
>> + comparisons for an identity dictionary."
>> +
>> + aSymbol == #==
>> + ifTrue: [ ^self privatePerform: aSymbol with: anObject ]
>> + ifFalse: [ ^package perform: aSymbol with: anObject "as per doesNotUnderStand:" ]
>> + !
>>
>> Item was added:
>> + ----- Method: MCPackageInEnvironment>>privatePerform:with: (in category 'delegating') -----
>> + privatePerform: aSymbol with: anObject
>> + "aSymbol is #== and the sender is trying to perform an identity comparison.
>> + Invoke primitivePerform."
>> +
>> + <primitive: 83>
>> + ^ self primitiveFailed!
>>
>>

> Hi Dave,
>
> I think the best would be if SystemTracer used the mirror primitives (See
> Context's mirror primitives method category), primitive 110 (#object:eqeq:)
> in this case, so that proxies like MCPackageInEnvironment can't affect it.
>
> Levente
>
> On Sun, 13 May 2018, David T. Lewis wrote:
>
>> This update feels a bit hacky to me with its test for a specific selector,
> so
>> better or clearer solutions are welcome. Meanwhile, the prior behavior was
>> corrupting identity dictionaries, which seems like a Bad Thing, so I put
>> this fix (along with a new unit test) directly into trunk.
>>
>> The problem showed up as I was trying to get SystemTracer64 working again
> on
>> V3 images, so it was not an easy thing to debug :-/
>>
>> Dave
>>
>> On Sun, May 13, 2018 at 11:14:39PM +0000, [hidden email] wrote:
>>> David T. Lewis uploaded a new version of Monticello to project The Trunk:
>>> http://source.squeak.org/trunk/Monticello-dtl.682.mcz
>>>
>>> ==================== Summary ====================
>>>
>>> Name: Monticello-dtl.682
>>> Author: dtl
>>> Time: 13 May 2018, 7:14:27.068827 pm
>>> UUID: 76fc84e6-7e2b-4a6a-af6b-2d02d11c38eb
>>> Ancestors: Monticello-nice.681
>>>
>>> Sending perform: #== with: anObject to an instance of
> MCPackageInEnvironment should test identity of the actual instance, not the
> MCPackage to which it refers. If this is not so, a PluggableDictionary will
> fail when doing identity checks on its keys, as is the case for a
> SystemTracer adding all objects to its oop dictionary.
>>>
>>> Implement MCPackageInEnvironment>>perform:with: such that performing #==
> will call primitivePerform, and all other selectors are delegated to the
> MCPackage as before.
>>>
>>> =============== Diff against Monticello-nice.681 ===============
>>>
>>> Item was added:
>>> + ----- Method: MCPackageInEnvironment>>perform:with: (in category
> 'delegating') -----
>>> + perform: aSymbol with: anObject
>>> + "If aSymbol is #== then the sender is trying to perform an identity
> comparison.
>>> + This cannot be delegated, because the delegate will be a different
> object.
>>> + Implement perform:with: here to protect for this case. An important
> example
>>> + is the case of PluggableDictionary, which relies on perform:with: to
> implement
>>> + comparisons for an identity dictionary."
>>> +
>>> + aSymbol == #==
>>> + ifTrue: [ ^self privatePerform: aSymbol with: anObject ]
>>> + ifFalse: [ ^package perform: aSymbol with: anObject "as per
> doesNotUnderStand:" ]
>>> + !
>>>
>>> Item was added:
>>> + ----- Method: MCPackageInEnvironment>>privatePerform:with: (in category
> 'delegating') -----
>>> + privatePerform: aSymbol with: anObject
>>> + "aSymbol is #== and the sender is trying to perform an identity
> comparison.
>>> + Invoke primitivePerform."
>>> +
>>> + <primitive: 83>
>>> + ^ self primitiveFailed!
>>>
>>>
>
>

> Having a look at the code, it's probably enough to change
> #newLargeIdentityDictionaryOfSize: to be
>
> ^(PluggableDictionary new: size)
> hashBlock: [ :object | object largeHash ];
> equalBlock: [ :a :b | a == b ];
> yourself
>
> That change should also make the code somewhat faster.
>
> Levente
>
> On Mon, 14 May 2018, Levente Uzonyi wrote:
>
> >Hi Dave,
> >
> >I think the best would be if SystemTracer used the mirror primitives (See
> >Context's mirror primitives method category), primitive 110
> >(#object:eqeq:) in this case, so that proxies like MCPackageInEnvironment
> >can't affect it.
> >
> >Levente
> >
> >On Sun, 13 May 2018, David T. Lewis wrote:
> >
> >>This update feels a bit hacky to me with its test for a specific
> >>selector,
> >so
> >>better or clearer solutions are welcome. Meanwhile, the prior behavior was
> >>corrupting identity dictionaries, which seems like a Bad Thing, so I put
> >>this fix (along with a new unit test) directly into trunk.
> >>
> >>The problem showed up as I was trying to get SystemTracer64 working again
> >on
> >>V3 images, so it was not an easy thing to debug :-/
> >>
> >>Dave
> >>
> >>On Sun, May 13, 2018 at 11:14:39PM +0000, [hidden email] wrote:
> >>>David T. Lewis uploaded a new version of Monticello to project The Trunk:
> >>>http://source.squeak.org/trunk/Monticello-dtl.682.mcz
> >>>
> >>>==================== Summary ====================
> >>>
> >>>Name: Monticello-dtl.682
> >>>Author: dtl
> >>>Time: 13 May 2018, 7:14:27.068827 pm
> >>>UUID: 76fc84e6-7e2b-4a6a-af6b-2d02d11c38eb
> >>>Ancestors: Monticello-nice.681
> >>>
> >>>Sending perform: #== with: anObject to an instance of
> >MCPackageInEnvironment should test identity of the actual instance, not
> >the MCPackage to which it refers. If this is not so, a PluggableDictionary
> >will fail when doing identity checks on its keys, as is the case for a
> >SystemTracer adding all objects to its oop dictionary.
> >>>
> >>>Implement MCPackageInEnvironment>>perform:with: such that performing #==
> >will call primitivePerform, and all other selectors are delegated to the
> >MCPackage as before.
> >>>
> >>>=============== Diff against Monticello-nice.681 ===============
> >>>
> >>>Item was added:
> >>>+ ----- Method: MCPackageInEnvironment>>perform:with: (in category
> >'delegating') -----
> >>>+ perform: aSymbol with: anObject
> >>>+ "If aSymbol is #== then the sender is trying to perform an identity
> >comparison.
> >>>+ This cannot be delegated, because the delegate will be a different
> >object.
> >>>+ Implement perform:with: here to protect for this case. An important
> >example
> >>>+ is the case of PluggableDictionary, which relies on perform:with: to
> >implement
> >>>+ comparisons for an identity dictionary."
> >>>+
> >>>+ aSymbol == #==
> >>>+ ifTrue: [ ^self privatePerform: aSymbol with: anObject ]
> >>>+ ifFalse: [ ^package perform: aSymbol with: anObject "as per
> >doesNotUnderStand:" ]
> >>>+ !
> >>>
> >>>Item was added:
> >>>+ ----- Method: MCPackageInEnvironment>>privatePerform:with: (in
> >>>category
> >'delegating') -----
> >>>+ privatePerform: aSymbol with: anObject
> >>>+ "aSymbol is #== and the sender is trying to perform an identity
> >comparison.
> >>>+ Invoke primitivePerform."
> >>>+
> >>>+ <primitive: 83>
> >>>+ ^ self primitiveFailed!
> >>>
> >>>
> >
> >
>

> Thanks Levente,
>
> Indeed that is a much simpler solution. The fact that the selector refers
> to "hashBlock" and "equalBlock" should have been a clue, thank you for
> pointing it out.
>
> Should I revert the changes? In testPerformIdentityEquals I wrote this:
>
> "Selector #== must refer to aProtoObj, not to the MCPackage to which it delegates"
> self assert: [aProtoObj perform: #== with: aProtoObj]
> description: 'If aProtoObj delegates to the MCPackage the identity test will fail'.
>
> because I expected that identity test should always refer to the actual receiver
> object, but maybe that is no the right assumption in the case of proxies.
>
> Dave
>
>
> On Mon, May 14, 2018 at 01:52:14AM +0200, Levente Uzonyi wrote:
>> Having a look at the code, it's probably enough to change
>> #newLargeIdentityDictionaryOfSize: to be
>>
>> ^(PluggableDictionary new: size)
>> hashBlock: [ :object | object largeHash ];
>> equalBlock: [ :a :b | a == b ];
>> yourself
>>
>> That change should also make the code somewhat faster.
>>
>> Levente
>>
>> On Mon, 14 May 2018, Levente Uzonyi wrote:
>>
>> >Hi Dave,
>> >
>> >I think the best would be if SystemTracer used the mirror primitives (See
>> >Context's mirror primitives method category), primitive 110
>> >(#object:eqeq:) in this case, so that proxies like MCPackageInEnvironment
>> >can't affect it.
>> >
>> >Levente
>> >
>> >On Sun, 13 May 2018, David T. Lewis wrote:
>> >
>> >>This update feels a bit hacky to me with its test for a specific
>> >>selector,
>> >so
>> >>better or clearer solutions are welcome. Meanwhile, the prior behavior was
>> >>corrupting identity dictionaries, which seems like a Bad Thing, so I put
>> >>this fix (along with a new unit test) directly into trunk.
>> >>
>> >>The problem showed up as I was trying to get SystemTracer64 working again
>> >on
>> >>V3 images, so it was not an easy thing to debug :-/
>> >>
>> >>Dave
>> >>
>> >>On Sun, May 13, 2018 at 11:14:39PM +0000, [hidden email] wrote:
>> >>>David T. Lewis uploaded a new version of Monticello to project The Trunk:
>> >>>http://source.squeak.org/trunk/Monticello-dtl.682.mcz
>> >>>
>> >>>==================== Summary ====================
>> >>>
>> >>>Name: Monticello-dtl.682
>> >>>Author: dtl
>> >>>Time: 13 May 2018, 7:14:27.068827 pm
>> >>>UUID: 76fc84e6-7e2b-4a6a-af6b-2d02d11c38eb
>> >>>Ancestors: Monticello-nice.681
>> >>>
>> >>>Sending perform: #== with: anObject to an instance of
>> >MCPackageInEnvironment should test identity of the actual instance, not
>> >the MCPackage to which it refers. If this is not so, a PluggableDictionary
>> >will fail when doing identity checks on its keys, as is the case for a
>> >SystemTracer adding all objects to its oop dictionary.
>> >>>
>> >>>Implement MCPackageInEnvironment>>perform:with: such that performing #==
>> >will call primitivePerform, and all other selectors are delegated to the
>> >MCPackage as before.
>> >>>
>> >>>=============== Diff against Monticello-nice.681 ===============
>> >>>
>> >>>Item was added:
>> >>>+ ----- Method: MCPackageInEnvironment>>perform:with: (in category
>> >'delegating') -----
>> >>>+ perform: aSymbol with: anObject
>> >>>+ "If aSymbol is #== then the sender is trying to perform an identity
>> >comparison.
>> >>>+ This cannot be delegated, because the delegate will be a different
>> >object.
>> >>>+ Implement perform:with: here to protect for this case. An important
>> >example
>> >>>+ is the case of PluggableDictionary, which relies on perform:with: to
>> >implement
>> >>>+ comparisons for an identity dictionary."
>> >>>+
>> >>>+ aSymbol == #==
>> >>>+ ifTrue: [ ^self privatePerform: aSymbol with: anObject ]
>> >>>+ ifFalse: [ ^package perform: aSymbol with: anObject "as per
>> >doesNotUnderStand:" ]
>> >>>+ !
>> >>>
>> >>>Item was added:
>> >>>+ ----- Method: MCPackageInEnvironment>>privatePerform:with: (in
>> >>>category
>> >'delegating') -----
>> >>>+ privatePerform: aSymbol with: anObject
>> >>>+ "aSymbol is #== and the sender is trying to perform an identity
>> >comparison.
>> >>>+ Invoke primitivePerform."
>> >>>+
>> >>>+ <primitive: 83>
>> >>>+ ^ self primitiveFailed!
>> >>>
>> >>>
>> >
>> >
>>