<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2018-04-17 16:43 GMT+02:00 Nicolas Cellier <span dir="ltr"><<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><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?</div></div></div></div></div></div></div></blockquote><div><br></div><div>Hmm, but I see several references to BytesPerWord from within ThreadedFFI...<br></div><div>Are you sure that it won't work?<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5"><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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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="gmail-m_5800437976428215237HOEnZb"><div class="gmail-m_5800437976428215237h5"><br>
> On Apr 17, 2018, at 5:38 AM, <a href="mailto:commits@source.squeak.org" target="_blank">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/VMMak<wbr>er/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-c6d212<wbr>25c848<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>>num32BitUni<wbr>tsOf: (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>>ffiPushStru<wbr>ctureContentsOf: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>
</div></div></blockquote></div><br></div></div>