[Vm-dev] FilePlugin argument checking - primitiveFailed vs PrimErrBadArgument

[Alistair Grant]

Hi Eliot,

On 1 March 2018 at 20:38, Eliot Miranda <eliot.miranda at gmail.com> wrote:
>
> Hi Alistair,
>
> On Thu, Mar 1, 2018 at 11:21 AM, Alistair Grant <akgrant0710 at gmail.com> wrote:
>>
>>
>> Hi Everyone,
>>
>> I'm currently extending FilePlugin to allow files to be opened using
>> either a file descriptor or FILE*.  If you're interested more details
>> at:
>>
>> Original PR: https://github.com/pharo-project/pharo-vm/pull/108
>> Updated PR: https://github.com/pharo-project/pharo-vm/pull/142
>>
>>
>> While tidying up the argument checking I noticed that FilePlugin has as
>> part of #primitiveFileOpen:
>>
>>
>>     namePointer := interpreterProxy stackValue: 1.
>>     (interpreterProxy isBytes: namePointer)
>>        ifFalse: [^ interpreterProxy primitiveFail].
>>
>>
>> My understanding is that this is bad practice as #primitiveFail won't
>> correctly retry if the argument is a forwarder.
>
>
> No, that's not so.  Any primitive failure is fine.  Remember that when we discussed this the FileAttributesPlugin was answering error codes as the result of a primitive instead of failing.  This is the approach that won't work.  What happens is that when a primitive fails (with any code including nil, the non-code used when failing via primitiveFail) is that the VM scans the parameters to the primitive, following forwarders, and retries the primitive if it finds any.  So there's no need to change primitiveFail into primitiveFailFor: PrimErrXXX.

Ah right, thanks for the clarification and reminder.


> However, good error codes can allow much nicer primitive failure code.  Testing the error code can be faster and simpler than interrogating the arguments to the primitive.  For example, here are two versions of ByteString>>at:put: that deal with read-only objects.
>
> at: index put: aCharacter
> "Primitive. Store the Character in the field of the receiver indicated by
> the index. Fail if the index is not an Integer or is out of bounds, or if
> the argument is not a Character. Essential. See Object documentation
> whatIsAPrimitive."
>
> <primitive: 64 error: ec>
> aCharacter isCharacter
> ifFalse:[^self errorImproperStore].
> aCharacter isOctetCharacter ifFalse:[
> "Convert to WideString"
> self becomeForward: (WideString from: self).
> ^self at: index put: aCharacter.
> ].
> index isInteger
> ifTrue: [ (index between: 1 and: self size)
> ifFalse: [ self errorSubscriptBounds: index ] ]
> ifFalse: [self errorNonIntegerIndex].
> self isReadOnlyObject
> ifTrue: [ ^ self modificationForbiddenFor: #at:put: index: index value: aCharacter ]
>
> and now testing the error code directly:
>
> at: index put: aCharacter
> "Primitive. Store the Character in the field of the receiver indicated by the index.
> Fail if the index is not an Integer or is out of bounds, or if the argument is not a
> Character, or the Character's code is outside the 0-255 range, or if the receiver
> is read-only. Essential. See Object documentation whatIsAPrimitive."
>
> <primitive: 64 error: ec>
> aCharacter isCharacter ifFalse:
> [^self errorImproperStore].
> index isInteger
> ifTrue:
> [ec == #'no modification' ifTrue:
> [^self modificationForbiddenFor: #at:put: index: index value: aCharacter].
> aCharacter isOctetCharacter ifFalse: "Convert to WideString"
> [self becomeForward: (WideString from: self).
> ^self at: index put: aCharacter].
> self errorSubscriptBounds: index]
> ifFalse: [self errorNonIntegerIndex]
>
>> So it should be:
>>
>>
>>     namePointer := interpreterProxy stackValue: 1.
>>     (interpreterProxy isBytes: namePointer)
>>        ifFalse: [^ interpreterProxy primitiveFailFor: PrimErrBadArgument].
>>
>>
>> Can someone confirm that I'm not misunderstanding, and I'll go through
>> and check FilePlugin further.
>
>
> Feel free to add better error codes, but do understand that it is orthogonal to the forwarder following issue.  The important thing is that primitives must fail, not answer error codes as results, i.e. that primitives be implemented as they always have been.

I'll add better error codes in the new primitives and leave existing
primitives unchanged.

Thanks again,
Alistair