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

[Eliot Miranda]

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.

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.

Cheers!

_,,,^..^,,,_
best, Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20180301/4c1406ec/attachment.html>