<div dir="ltr"><div><div><div><div><div><div><div>Hi Eliot,<br>
<div>First, sorry for multiple posting...<br></div>I've been fooled by repeated ConnectionTimedOut windows.<br><br></div>Then, you must be right I've been away from VMMaker for too long...<br>Maybe we could use (self sizeof: sqintptr_t) or something like that?<br><br></div>As for the need of rewrite, yes!
<div>I've tried to reverse-engineer all the logic in ThreadedFFIPlugin...<br></div>It's far from simple, and several combinations behave like I don't expect...<br>(but I'm not completely sure, the best way to confirm would be to simulate some TestCase).<br><br></div>What I understand is joined in the .xls attachment.<br></div><div>Left columns A-E are the status of formal parameter spec (3 bits of argType compiledSpec, and status of argType referentClass which can be either nil or some subclass of ExternalStructure)<br></div><div>Several combinations are valids:<br>- Basic atomic types and corresponding pointer types have a referentClass nil.<br></div><div>- Named atomic types aliases have referentClass being an ExternalStructure.<br></div><div>- Named structure types have
referentClass being an ExternalStructure, unless they are undefined...<br><br></div><div>Column F is the secondary ffi method used to marshal the actual argument<br></div><div>Columns G-Q are various possible types for either the Smalltalk argument, or its handle in case of ExternalStruct.<br></div><div>I've omitted additional case of Alien (direct/indirect) and WordArray, so this is incomplete.<br></div><div><br></div>I have not made all the necessary conditions explicit...<br>For example we may have several ways to check types:<br></div>- thru equal compiledSpec (or mostly equal, except last bit for signed/unsigned atomic type tolerance)<br></div>- thru inherited referentClass<br></div><div><br><img src="cid:ii_jg3r8ghe0_162d3f9cadb1a84f" height="411" width="562"><br><br></div><div>What I would like is more like this:<br><br></div><div><img src="cid:ii_jg3rg7rg1_162d3ff538383db1" height="412" width="562"><br><br></div>Not sure how the figure will pass thru the ML, but see attachment.<br><div><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">2018-04-17 16:11 GMT+02:00 Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
Hi Nicolas,<br>
<br>
let's at least try and refactor so the validity checks are performed in methods with intention revealing selectors so we can more easily understand the code. Alas I think your approach below won't work because it adds to the sqVirtualMachine Interpreter proxy interface. Instead you could try implementing num32BitUnitsOf: in ThreadedFFIPlugin and use byteSizeOf:.<br>
<br>
_,,,^..^,,,_ (phone)<br>
<div class="HOEnZb"><div class="h5"><br>
> On Apr 17, 2018, at 5:38 AM, <a href="mailto:commits@source.squeak.org">commits@source.squeak.org</a> wrote:<br>
> <br>
> <br>
> Nicolas Cellier uploaded a new version of VMMaker to project VM Maker:<br>
> <a href="http://source.squeak.org/VMMaker/VMMaker.oscog-" rel="noreferrer" target="_blank">http://source.squeak.org/<wbr>VMMaker/VMMaker.oscog-</a> nice.2366.mcz<br>
> <br>
> ==================== Summary ====================<br>
> <br>
> Name: VMMaker.oscog- nice.2366<br>
> Author: nice<br>
> Time: 17 April 2018, 3:37:48.684079 pm<br>
> UUID: 6d9bd2fa-c381-5d4b-b31e-<wbr>c6d21225c848<br>
> Ancestors: VMMaker.oscog-VB.2365<br>
> <br>
> Correct a 32bit-hardcoded pointer size in FFI<br>
> Correct two copy/paste typos in num32BitUnitsOf:<br>
> <br>
> Note: I don't like the FFI code that I just corrected. IMO, it does the wrong thing.<br>
> <br>
> if I have an argument spec is<br>
> MyLib>>foo: aFoo<br>
> <cdecl: void foo(Foo *)><br>
> where Foo is some ExternalStructure subclass (Foo class>>fields ^#((x 'ushort') (y 'ushort')))<br>
> <br>
> and that I try to pass (MyLib new foo: Foo new), it seems to me that the Foo new getHandle will be (ByteArray new: 4).<br>
> What I understand form the code that I just corrected is that we are trying to pass the contents of the ByteArray re-interpreted as a void pointer. Scary and wrong...<br>
> <br>
> If I instead pass (MyLib new foo: Foo externalNew), it seems that we don't even bother to check if the (argSpec anyMask: FFIFlagPointer) and just force passing the structure by value (thru a memcpy on stack). Scary and wrong...<br>
> <br>
> In general, every one use <cdecl: void foo(void *)> to work around this ill-behavior, and thus bypass type checks...<br>
> <br>
> Also note that we can't even pass an ExternalData (think an Array of Foo), because ffiArgument:Spec:Class:in: insists on having actualArg class inheritsFrom: argType referentClass. ExternalData does not inherit from Foo, event if its type matches (ExternalType structTypeNamed: #Foo). That's crazy...<br>
> Another reason while people use <cdecl: void foo(void *)><br>
> <br>
> It's high time to consider a rewrite IMO.<br>
> <br>
> =============== Diff against VMMaker.oscog-VB.2365 ===============<br>
> <br>
> Item was changed:<br>
> ----- Method: ObjectMemory>>num32BitUnitsOf: (in category 'object access') -----<br>
> num32BitUnitsOf: objOop <br>
> + "Answer the number of 32-bit units in the given non-immediate object.<br>
> - "Answer the number of 16-bit units in the given non-immediate object.<br>
> N..B. Rounds down 8-bit units, so a 7 byte object has 1 32-bit unit.<br>
> Does not adjust the size of contexts by stackPointer."<br>
> ^(self numBytesOf: objOop) >> 2!<br>
> <br>
> Item was changed:<br>
> ----- Method: SpurMemoryManager>><wbr>num32BitUnitsOf: (in category 'object access') -----<br>
> num32BitUnitsOf: objOop <br>
> + "Answer the number of 32-bit units in the given non-immediate object.<br>
> - "Answer the number of 16-bit units in the given non-immediate object.<br>
> N..B. Rounds down 8-bit units, so a 7 byte object has 1 32-bit unit.<br>
> Does not adjust the size of contexts by stackPointer."<br>
> ^(self numBytesOf: objOop) >> 2!<br>
> <br>
> Item was changed:<br>
> ----- Method: ThreadedFFIPlugin>><wbr>ffiPushStructureContentsOf:in: (in category 'callout support') -----<br>
> ffiPushStructureContentsOf: oop in: calloutState<br>
> <var: #calloutState type: #'CalloutState *'><br>
> "Push the contents of the given external structure"<br>
> | ptrClass ptrAddress |<br>
> <inline: true><br>
> ptrClass := interpreterProxy fetchClassOf: oop.<br>
> ptrClass = interpreterProxy classExternalAddress ifTrue: "ExternalAddress is bytes"<br>
> [ptrAddress := (interpreterProxy fetchPointer: 0 ofObject: oop) asVoidPointer.<br>
> "There is no way we can make sure the structure is valid.<br>
> But we can at least check for attempts to pass pointers to ST memory."<br>
> (interpreterProxy isInMemory: ptrAddress) ifTrue:<br>
> [^FFIErrorInvalidPointer].<br>
> ^self ffiPushStructure: ptrAddress<br>
> ofSize: (calloutState ffiArgHeader bitAnd: FFIStructSizeMask)<br>
> typeSpec: calloutState ffiArgSpec<br>
> ofLength: calloutState ffiArgSpecSize<br>
> in: calloutState].<br>
> ptrClass = interpreterProxy classByteArray ifTrue:<br>
> ["The following is a somewhat pessimistic test but I like being sure..."<br>
> (interpreterProxy byteSizeOf: oop) = (calloutState ffiArgHeader bitAnd: FFIStructSizeMask)<br>
> ifFalse:[^FFIErrorStructSize].<br>
> ptrAddress := interpreterProxy firstIndexableField: oop.<br>
> (calloutState ffiArgHeader anyMask: FFIFlagPointer) ifFalse:<br>
> "Since this involves passing the address of the first indexable field we need to fail<br>
> the call if it is threaded and the object is young, since it may move during the call."<br>
> [self cppIf: COGMTVM ifTrue:<br>
> [((calloutState callFlags anyMask: FFICallFlagThreaded)<br>
> and: [interpreterProxy isYoung: oop]) ifTrue:<br>
> [^PrimErrObjectMayMove negated]].<br>
> ^self ffiPushStructure: ptrAddress<br>
> ofSize: (calloutState ffiArgHeader bitAnd: FFIStructSizeMask)<br>
> typeSpec: calloutState ffiArgSpec<br>
> ofLength: calloutState ffiArgSpecSize<br>
> in: calloutState].<br>
> "If FFIFlagPointer + FFIFlagStructure is set use ffiPushPointer on the contents"<br>
> + (calloutState ffiArgHeader bitAnd: FFIStructSizeMask) = BytesPerWord ifFalse:<br>
> - (calloutState ffiArgHeader bitAnd: FFIStructSizeMask) = 4 ifFalse:<br>
> [^FFIErrorStructSize].<br>
> ptrAddress := (interpreterProxy fetchPointer: 0 ofObject: oop) asVoidPointer.<br>
> (interpreterProxy isInMemory: ptrAddress) ifTrue:<br>
> [^FFIErrorInvalidPointer].<br>
> ^self ffiPushPointer: ptrAddress in: calloutState].<br>
> ^FFIErrorBadArg!<br>
> <br>
</div></div></blockquote></div><br></div>