[Vm-dev] Bug in #instVarAt:put: for read-only objects

Wed Jan 10 10:15:17 UTC 2018

Hi Eliot.

I definitely sure that nil parameter smells very bad:

Object>>noModificationErrorFor: selector value: value
^(NoModificationError receiver: self selector: selector *index: nil* value:
value) raiseRequest

Because in that case all users will be forced to check nils if they are
interesting only about field modification.
I can imaging such library which records history of object changes. It will
need exact field index which was modified.
Also ORM frameworks needs to handle differently field modification and
class modification of objects. Because new class can be mapped in different
table and such update will require complex logic. It would be natural to
dispatch concrete logic by error classes.

So I would introduce little hierarchy which I already mentioned:

ModificationForbidden
   FieldModificationForbidden
   ClassModificationForbidden
   BecomeModificationForbidden

On the base class there will be object, newValue, retrySelector and
isMirror flag. And fieldIndex will be moved to the
FieldModificationForbidden.
Mirror flag is required to correctly retry mirror based modification
because first argument in that case is always "actual receiver" instead of
index.

I split class and become based modifications because it is logically
different errors. Maybe it is not important distinction from the first look
but at least it is easy to name them separately. Do we have other cases
which are not related to field modification?

Also I have question about become which I sent to VM list.

Best regards,
Denis

2018-01-06 19:45 GMT+01:00 Eliot Miranda <eliot.miranda at gmail.com>:

>
> Hi Norbert, Hi Denis,
>
> On Sat, Jan 6, 2018 at 4:06 AM, Norbert Hartl <norbert at hartl.name> wrote:
>
>>
>>
>>
>> Am 05.01.2018 um 16:53 schrieb Clément Bera <bera.clement at gmail.com>:
>>
>>
>>
>> On Fri, Jan 5, 2018 at 4:05 PM, Norbert Hartl <norbert at hartl.name> wrote:
>>
>>>
>>> You mean
>>>
>>> <primitive: 174 error: ec>
>>> self isReadOnlyObject
>>> ifTrue: [(ModificationForbidden for: self atInstVar: index with:
>>> anObject) signal]
>>> ifFalse: [ self primitiveFailed ]
>>> ?
>>>
>>> Norbert
>>>
>>
>> yes something like that.
>>
>>
>> Same thing for Object>>#at:put:, floatAt:put: and so on.
>>
>>
>> I’m working on it and came by
>>
>> Object>>#primitiveChangeClassTo:
>>
>> and I wonder what would be the approach here. The
>> ModificationForbidden/NoModificationError seems to be tight to an index.
>> What would be the case for a class change? Another Exception class? How is
>> that solved in VW?
>>
>
> I don't think that adding variants of NoModificationError is wise.  One
> can imagine adding other state changes that read-onliness should prevent,
> such as unpinning.  So one could end up having to add a lot of subclasses
> of NoModificationError, each rather inflexible and able to respond to only
> one specific error.  We would end up with at least
> InstanceVariableModificationError IndexedVariableModificationError
> ClassModificationError BecomeIdentotyChangeError etc.  And even then
> IndexedVariableModificationError would need a message since there are
> many different kinds of at:put:s, at:put:, byteAt:put: unsignedLongAt:put:
> etc.
>
> In VW it is handled by the NoModificationError holding a Message that can
> be used for the retry operation.  i.e.
>
> Object>>noModificationErrorFor: selector index: index value: value
> ^(NoModificationError receiver: self selector: selector index: index
> value: value) raiseRequest
>
> (N.B. the following is sent by the VM when an inst var assignment fails
> due to read-onlyness; see recreateSpecialObjectsArray)
> Object>>attemptToAssign: aValue withIndex: index
> ^self noModificationErrorFor: #instVarAt:put: index: index value: aValue
>
> Object>>at: index put: value
> "Store the argument value in the indexable field of the receiver indicated
> by
> index. Fail if the index is not an Integer or is out of bounds. Fail if the
> value is not of the right type for this kind of collection. Answer the
> value that was stored."
>
> <primitive: 61 errorCode: ec>
> index isInteger ifTrue:
> [(index >= 1 and: [index <= self basicSize])
> ifTrue:
> [self isImmutable
> ifTrue: [^self noModificationErrorFor: #at:put: index: index value: value]
> ifFalse: [^self improperStoreError]]
> ifFalse: [^self subscriptBoundsErrorFor: #at:put: index: index value:
> value]].
> index respondsToArithmetic
> ifTrue: [^self at: index asSmallInteger put: value]
> ifFalse: [^self nonIntegerIndexError: index]
>
> Behavior>>adoptInstance: anObject
> "Changes the class of the argument to be that of the receiver provided that
>  a) anObject is not immutable (which includes immediates)
>  b) anObject is bits and the receiver is a bits class, or
>  c) anObject is pointers and the receiver is pointers and anObject has
> the same
> number of inst vars as specified by the receiver, or the receiver is
> variable and
> anObject has at least as many inst vars as specified by the receiver.
> Answers the receiver."
>
> <primitive: 536 errorCode: errCode>
> ^(errCode ~~ nil
>   and: [errCode name = #'no modification'])
> ifTrue: [anObject noModificationErrorFor: #changeClassTo: index: nil
> value: self]
> ifFalse: [self primitiveFailed]
>
> etc.  This is much more flexible and works well in practice.  In the
> vw7.7nc image I looked at there are 20 senders of
> #noModificationErrorFor:index:value: for variants of 5 operations:
>
> - attempting to assign to an inst var of a read-only object, either
> directly in code via an inst var assignment or via an instVarAt:put: send
> - attempting to assign to an indexed inst var of a read-only object, via
> an at:put: send (at:put: byteAt:put: unsignedLongAt:put: etc)
> - attempting to assign to an indexed inst var of a read-only object via a
> BitBlt primitive (e.g. copyBitsClippedStride:
> width:atX:y:from:stride:width:atX:y:width:height:rule:)
> - attempting to become a read-only object via two-way become; forwarding
> become is not considered a modification.
> - attempting to change the class of a read-only object
>
>
>
> Digression:
>
> Looking at the above the obvious convenience is to add
>
> Object>>noModificationErrorFor: selector value: value
> ^(NoModificationError receiver: self selector: selector index: nil value:
> value) raiseRequest
>
> which would be applicable in 8 of the 20 uses, but it would imply adding
> overrides in subclasses.  #noModificationErrorFor:index:value: is
> implemented in ProtoObect, Object, Symbol and VariableBinding. The
> implementations in Symbol and VariableBinding simply provide more
> explanatory error messages.
>
> HTH
>
>
>> Norbert
>>
>>
>>
>>>
>>> Am 05.01.2018 um 14:22 schrieb Clément Bera <bera.clement at gmail.com>:
>>>
>>> Hi,
>>>
>>> No this is not a bug.
>>>
>>> This needs to be handled in the primitive failure code in the image, the
>>> method should be something like that :
>>>
>>> Object>>instVarAt: index put: anObject
>>> <primitive: 174 error: ec>
>>>         self isReadOnlyObject ifTrue: [(ModificationForbidden for: self
>>> atInstVar: index with: anObject) signal]
>>> self primitiveFailed
>>>
>>> All primitive fall-back code triggering object mutation should be
>>> rewritten this way, especially primitives such as #at:put:,
>>> #instVarAt:put:, etc.
>>>
>>> Cheers
>>>
>>> On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <norbert at hartl.name> wro
>>> te:
>>>
>>>>
>>>> If I do
>>>>
>>>> (#foo -> #bar)
>>>> beReadOnlyObject;
>>>> value: #baz
>>>>
>>>> it throws
>>>>
>>>> *ModificationForbidden:  #foo->#bar is read-only, hence its field 2
>>>> cannot be modified with #baz*
>>>>
>>>>  which is as expected. But if I do
>>>>
>>>> (#foo -> #bar)
>>>> beReadOnlyObject;
>>>> instVarNamed: #value put: #baz
>>>>
>>>> I get
>>>>
>>>> *PrimitiveFailed: primitive #instVarAt:put: in Association failed*
>>>>
>>>> I think this a bug, no?
>>>>
>>>> Norbert
>>>>
>>>>
>>>
>>>
>>> --
>>> Clément Béra
>>> Pharo consortium engineer
>>> https://clementbera.wordpress.com/
>>> Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
>>>
>>>
>>>
>>>
>>
>>
>> --
>> Clément Béra
>> Pharo consortium engineer
>> https://clementbera.wordpress.com/
>> Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
>>
>>
>>
>>
>
>
> --
> _,,,^..^,,,_
> best, Eliot
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20180110/65befa60/attachment-0001.html>