<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le lun. 22 juin 2020 à 11:50, Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com">nicolas.cellier.aka.nice@gmail.com</a>> a écrit :<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 dir="ltr"><div>Hi Marcel,</div><div>I think we have difficulties to understand each other, while I'm pretty sure that we mostly agree (except on 1 little detail).</div><div><br></div><div>I decided to refactor the FFI Plugin because it reached the point to being</div><div>- incomprehensible</div><div>- way too complex</div><div>- impossible to maintain/change</div><div><br></div><div>Take a look at the combinatorial  on the parameter specification side:<br></div><div>Atomic + pointer + struct + referentClass isNil</div><div>That's roughly 16 possibilities</div><div><br></div><div>Take a look at the combinatorial of actual arguments:</div><div>ByteArray ExternalAddress Alien WordArray immediate values (char int float) nil,true,false<br></div><div>ExternalStructure ExternalAlias ExternalData</div><div><br></div><div>Now draw a map of what actual argument you can pass to what specification and what you cannot.</div><div>It's the Excel that I sent to you. I had to decipher that sheet from reverse engineering...</div><div>Way too complex! We reached the limit and MUST refactor (or die).<br></div><div><br></div><div>So what I'm proposing is essentially a refactoring, and almost a statu quo (except the ones marked new!)<br></div><div>I want to reduce the number of combinations to 6 instead of 16, the 6 that makes sense IMO.<br></div><div>And at the same time, I want to extend the possible actual parameters, and ensure type safety (though it's more for future once we get pointer arity).<br></div><div><br></div><div>The 6 combinations that make sense are:<br></div><div>1) atomic <-> immediate value (or type alias)</div><div>2) atomic + referentClass <-> ExternalTypeAlias or ExternalData (new!)</div><div><div>3) atomic pointer <-> ExternalData or direct ByteArray, WordArray, ExternalAddress, Alien (no change, is required for UFFI, Alien etc...) DoubleByte/WordArray (new!)<br></div><div>4) atomic pointer + referentClass <-> ExternalData</div><div>5) structure + referentClass <-> ExternalStructure or ExternalData (new!)</div><div>6) structure + pointer + referentClass <-> ExternalStructure (new!) or ExternalData</div><div><br></div><div>The refactoring makes those 6 possibilities very clear by virtue of #caseOf:</div><div>There is one method ffiPassArgument* for each of the 6 possibilities.<br></div></div><div><br></div><div>What are the essential changes?<br></div><div>1) allow passing an ExternalData when we specified a parameter by value</div><div>This is a convenience for supporting global variables.</div><div>A global variable is a reference (an ExternalAddress) and a type incarnated in an ExternalData.</div><div>We want to be able to pass a global variable as argument to an external function call (whether by value or by reference)<br></div><div>So either the marshalling of objects is performed at image side (see UFFI for example, or somehow DLLCC in VW).</div><div>Or it is performed by the plugin itself (the case of SqueakFFI).</div><div>You proposed a mixed way via doesNotCoerce:. This can be interesting but more complex IMO (vs UFFI).</div><div><br></div></div></blockquote><div>Ah one more thing,</div><div>I think that image side marshalling is much more powerful, because extensible.</div><div>Yes, it's far easier to coerce at image side, extend with new classes, etc...</div><div>So, IMO, UFFI is on the right track.<br></div><div><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 dir="ltr"><div></div><div>2) have a CLEAR encoding of combinations</div><div> That means adding structure + pointer flag to pointer to ExternalStruct (if not atomic type alias).</div><div> You legitimately ask whether it's needed to distinguish atomic+pointer+referentClass from structure+pointer+referentClass</div><div> We might review that. For now, it enables passing an ExternalStruct directly to a struct pointer without resorting to an ExternalData.<br></div><div>  If you further look at the combinations, obviously, we ain't gonna need to flags struct + atomic. They are exclusive. So yes, this refactoring is only a first step to have things clarified. Then we can drive further.<br></div><div><br></div><div>I perfectly understand the importance of ExternalData and how it is central in the FFI and the relations to ExternalType.</div><div>I simply disagree on the urge to IMPOSE ExternalData for every interaction with our domain types/objects.</div><div>We can expose, but should not impose.<br></div><div>ExternalData can be used both for scalar or arrays. It's conceptually a ValueHolder. </div><div>We cannot really differentiate a pointer to a single value or to an array of values... This is the nature of C.</div><div>ExternalStructure is hybrid. It acts both as</div><div>- an ExternalData (the handle is the same as a scalar ExternalData)</div><div>- and as a type surrogate (by name in function interface specification, and via referentClass in the plugin)</div><div>If you take the POV of a user of FFI rather than the one of implementer of FFI, you'll see that this is the central object that user may and want to deal with.</div><div>That's our only point of friction I think, that should better be discussed around a beer!</div><div><br></div><div>cheers<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le lun. 22 juin 2020 à 10:44, Marcel Taeumel <<a href="mailto:marcel.taeumel@hpi.de" target="_blank">marcel.taeumel@hpi.de</a>> a écrit :<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 id="gmail-m_3837403443535718051gmail-m_-4785883682421147801__MailbirdStyleContent" style="font-size:10pt;font-family:Arial;color:rgb(0,0,0)">
                                        
                                        
                                            
                                        
                                        
                                        Let me add that I am really surprised (and a little bit worried) that this little requirement of "wrap alias to atomic into referentClass on return" turned into such a huge proposal of yours.<div><br></div><div>All that other coercing stuff could easily be added via #doesNotCoerce: as proposed here: </div><div><a href="http://forum.world.st/FFI-Plugin-Proposal-Add-doesNotCoerce-for-like-doesNotUnderstand-td5118724.html" target="_blank"><span style="font-size:10pt">http://forum.world.st/FFI-Plugin-Proposal-Add-doesNotCoerce-for-like-doesNotUnderstand-td5118724.html</span></a><br><div><br></div><div>Please, try to understand the relationship between ExternalData and ExternalStructure/Union/TypeAlias. It's so simple. But maybe not so clear. ExternalData is not an implementation detail. It's part of the public interface; it's like Array but as a composition of handle and type. And maybe size and offset -- but that's due to heap memory management part.</div><div><br></div><div>Type safety is valuable, of course. Maybe you can give more examples of what you want to achieve. :-)</div><div><br></div><div>Best,</div><div>Marcel</div></div><div></div>
                                        
                                        <blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-top:20px;margin-left:0px;padding-left:10px;min-width:500px">
                        <p style="color:rgb(170,170,170);margin-top:10px">Am 22.06.2020 10:31:47 schrieb Marcel Taeumel <<a href="mailto:marcel.taeumel@hpi.de" target="_blank">marcel.taeumel@hpi.de</a>>:</p><div style="font-family:Arial,Helvetica,sans-serif"><div id="gmail-m_3837403443535718051gmail-m_-4785883682421147801__MailbirdStyleContent" style="font-size:10pt;font-family:Arial;color:rgb(0,0,0)">
                                        > <span style="font-family:Arial,Helvetica,sans-serif;font-size:13px">throw that code away, and have a look at  in VMMakerInbox, you will understand better...</span><div><span style="font-family:Arial,Helvetica,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:Arial,Helvetica,sans-serif;font-size:13px">Not so sure ... because I think we do not agree on the status quo yet. We should do that first. Understand the compiledSpec as is. Then move forward.</span></div><div><span style="font-family:Arial,Helvetica,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:Arial,Helvetica,sans-serif;font-size:13px">> </span><span style="font-family:Arial,Helvetica,sans-serif;font-size:13px">So hopefully, we shall throw away the FFIFlagStructure and make your life easier at image side :)</span></div><div><span style="font-family:Arial,Helvetica,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:Arial,Helvetica,sans-serif;font-size:13px">Not so sure. See above. </span></div><div><span style="font-family:Arial,Helvetica,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:Arial,Helvetica,sans-serif"><span style="font-size:13px">Best,</span></span></div><div><span style="font-family:Arial,Helvetica,sans-serif"><span style="font-size:13px">Marcel</span></span></div><div></div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-top:20px;margin-left:0px;padding-left:10px;min-width:500px">
                        <p style="color:rgb(170,170,170);margin-top:10px">Am 22.06.2020 10:28:12 schrieb Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>>:</p><div style="font-family:Arial,Helvetica,sans-serif">
<div dir="ltr"><div>No, no,</div><div>throw that code away, and have a look at  in VMMakerInbox, you will understand better...</div><div><br></div><div>In fact, if you read my proposals, I'd like to get rid of FFIFlagStructure altogether...</div><div>Either a type is atomic or composite, it's not both nor neither...</div><div>So we ain't gonna need 2 flags FFIFlagAtomic and FFIFlagStructure.</div><div><br></div><div>So hopefully, we shall throw away the FFIFlagStructure and make your life easier at image side :)<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le lun. 22 juin 2020 à 10:19, Marcel Taeumel <<a href="mailto:marcel.taeumel@hpi.de" target="_blank">marcel.taeumel@hpi.de</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex;min-width:500px"><div id="gmail-m_3837403443535718051gmail-m_-4785883682421147801gmail-m_3491715305796571077__MailbirdStyleContent" style="font-size:10pt;font-family:Arial;color:rgb(0,0,0)">
                                        
                                        
                                            
                                        
                                        
                                        Hi Nicolas,<div><br></div><div>sure, there might be a difference. But you have all the information you need directly in the plugin already:</div><div><br></div><div><img id="gmail-m_3837403443535718051gmail-m_-4785883682421147801gmail-m_3491715305796571077bb19c456-0c4e-450b-a8d3-2479268b49e5" src="cid:172db3f9d42cb971f161" width="auto"><br></div><div><br></div><div>Let me catch up on this later this week.</div><div><br></div><div>Best,</div><div>Marcel</div><div></div>
                                        
                                        <blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-top:20px;margin-left:0px;padding-left:10px;min-width:500px">
                        <p style="color:rgb(170,170,170);margin-top:10px">Am 22.06.2020 09:56:05 schrieb Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>>:</p><div style="font-family:Arial,Helvetica,sans-serif">
<div dir="ltr"><div>Hi Marcel,</div><div>So you mean that I don't really need to distinguish pointer to struct and pointer to atomic type alias?</div><div>Maybe...</div><div><br></div><div>There is currently a difference:</div><div>- an atomic type alias has an immediate value as handle</div><div>  it thus cannot be passed as parameter by reference</div><div>  (well, for aliases to pointers, I don't really know...)<br></div><div>- a struct has a memory zone as value (ExternalAddress or direct ByteArray)</div><div> it thus can be passed as parameter by reference without having to resort to an ExternalData.</div><div>See ffiPassStructureArgumentByReference: oop Class: oopClass expectedClass: argClass In: calloutState</div><div>vs ffiPassAtomicArgumentByReference: oop Class: oopClass expectedClass: argClass In: calloutState</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le lun. 22 juin 2020 à 09:18, Marcel Taeumel <<a href="mailto:marcel.taeumel@hpi.de" target="_blank">marcel.taeumel@hpi.de</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex;min-width:500px"><div id="gmail-m_3837403443535718051gmail-m_-4785883682421147801gmail-m_3491715305796571077gmail-m_5745492037820762996__MailbirdStyleContent" style="font-size:10pt;font-family:Arial;color:rgb(0,0,0)">
                                        
                                        
                                            
                                        
                                        
                                        Hi Nicolas,<div></div>
                                        
                                        <div><br></div><div>thanks! :-) <span style="font-size:10pt">I will take a look at it during this week, I hope. </span></div><div><span style="font-size:10pt"><br></span></div><div><span style="font-size:10pt">Here is a first thought:</span></div><div><span style="font-size:10pt"><br></span></div><div><span style="font-size:10pt">I don't think that the pointer types for external structs should have the FFIFlagStructure."referentClass" should be more than enough for the FFI plugin side for both coercing and return-value packaging.</span></div><div><span style="font-size:10pt"><br></span></div><div><span style="font-size:10pt">So, -1 for now but maybe I overlooked a use case. Raising this FFIFlagStructure here for such pointer types really messes up a lot of my current conceptual model about the relationship of ExternalStructure and ExternalType/ExternalStructureType. :-)</span></div><div><span style="font-size:10pt"><br></span></div><div><span style="font-size:10pt">Why is "referentClass" not enough for the plugin side? Just check for ifNil, if not, instantiate and put instVar 0 to the return value. Done. ;-) For coercing, just compare argument class with "referentClass" in the argType; then check for the pointer flag. Maybe discriminate between ByteArray and ExternalAddress. </span></div><div><span style="font-size:10pt"><br></span></div><div><span style="font-size:10pt">...please don't raise FFIFlagStructure for pointer types for external structures... pretty please ^__^</span></div><div><span style="font-size:10pt"><br></span></div><div><span style="font-size:10pt">Best,</span></div><div><span style="font-size:10pt">Marcel</span></div><div><span style="font-size:10pt"><br></span></div><blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-top:20px;margin-left:0px;padding-left:10px;min-width:500px">
                        <p style="color:rgb(170,170,170);margin-top:10px">Am 21.06.2020 22:34:09 schrieb <a href="mailto:commits@source.squeak.org" target="_blank">commits@source.squeak.org</a> <<a href="mailto:commits@source.squeak.org" target="_blank">commits@source.squeak.org</a>>:</p><div style="font-family:Arial,Helvetica,sans-serif">Nicolas Cellier uploaded a new version of FFI-Kernel to project FFI Inbox:<br><a href="http://source.squeak.org/FFIinbox/FFI-Kernel-nice.119.mcz" target="_blank">http://source.squeak.org/FFIinbox/FFI-Kernel-nice.119.mcz</a><br><br>==================== Summary ====================<br><br>Name: FFI-Kernel-nice.119<br>Author: nice<br>Time: 21 June 2020, 10:34:00.334284 pm<br>UUID: 3f2eca9d-8f55-476c-a0b0-c5ed6368a6b4<br>Ancestors: FFI-Kernel-nice.118<br><br>Make the compiledSpecs of struct pointers conform to the experimental FFI branch (thru #adjustPointerType)<br><br>See <a href="https://github.com/OpenSmalltalk/opensmalltalk-vm/tree/experimental_FFI" target="_blank">https://github.com/OpenSmalltalk/opensmalltalk-vm/tree/experimental_FFI</a><br><br>Simplify a bit ExternalData access (at: / at:put:).<br>The assumption that ExternalDataType is the type of the contents and not the type of the reference helps simplifying IMO.<br><br>We might want to make it more complete once we deal with pointer arity.<br>It's not the case yet.<br><br>=============== Diff against FFI-Kernel-nice.118 ===============<br><br>Item was changed:<br>  ----- Method: ExternalData>>at: (in category 'accessing') -----<br>  at: index<br>  <br>-    self<br>-                 assert: [index = 1 or: [type isAtomic]]<br>-              description: 'Should not read non-atomic pointer as array'.<br>- <br>       ((1 > index) or: [size notNil and: [index > size]])<br>             ifTrue: [^ self errorSubscriptBounds: index].<br>  <br>+    ^ type<br>-       ^ type asNonPointerType<br>               handle: handle<br>+               at: ((index-1) * type byteSize) + 1!<br>-                 at: ((index-1) * type asNonPointerType byteSize) + 1!<br><br>Item was changed:<br>  ----- Method: ExternalData>>at:put: (in category 'accessing') -----<br>  at: index put: value<br>  <br>-  self<br>-                 assert: [index = 1 or: [type isAtomic]]<br>-              description: 'Should not read non-atomic pointer as array'.<br>- <br>       ((1 > index) or: [size notNil and: [index > size]])<br>             ifTrue: [^ self errorSubscriptBounds: index].<br>  <br>+    ^ type<br>-       ^ type asNonPointerType<br>               handle: handle<br>+               at: ((index-1) * type byteSize) + 1<br>-          at: ((index-1) * type asNonPointerType byteSize) + 1<br>                  put: value!<br><br>Item was changed:<br>  ----- Method: ExternalStructure class>>compileAllFields (in category 'system startup') -----<br>  compileAllFields<br>    "<br>        ExternalStructure compileAllFields<br>    "<br>+       | priorAuthorInitials |<br>-      | priorAuthorInitials fieldSpec |<br>     priorAuthorInitials := Utilities authorInitialsPerSe.<br>         [Utilities setAuthorInitials: 'FFI'.<br>          <br>              self allStructuresInCompilationOrder do: [:structClass |<br>+                     | fieldSpec |<br>                         fieldSpec := structClass fields.<br>                      self flag: #discuss. "mt: Why do we need that extra layout check? Performance gain is minimal..."<br>                   (structClass hasFieldLayoutChanged: fieldSpec)<br>                                ifTrue: [structClass compileFieldsSilently: fieldSpec].<br>                       structClass externalType "asNonPointerType"<br>                                 compiledSpec: structClass compiledSpec;<br>+                              byteAlignment: structClass byteAlignment;<br>+                            adjustPointerType.<br>-                           byteAlignment: structClass byteAlignment.<br>                     structClass organization removeEmptyCategories].<br>              "Compilation of fields only needs external types temporarily. Non-weak references to external types are only in methods with FFI calls."<br>            ExternalType cleanupUnusedTypes.<br>  <br>          ] ensure: [Utilities setAuthorInitials: priorAuthorInitials]!<br><br>Item was added:<br>+ ----- Method: ExternalStructureType>>adjustPointerType (in category 'private') -----<br>+ adjustPointerType<br>+  self isPointerType<br>+           ifFalse: [self asPointerType<br>+                                 compiledSpec: (WordArray with: ((self compiledSpec first<br>+                                             bitAnd: FFIFlagAtomic + FFIFlagStructure)<br>+                                            bitOr: self class pointerSpec));<br>+                              byteAlignment: self class pointerAlignment]!<br><br>Item was changed:<br>  ----- Method: ExternalType class>>initializeStructureTypes (in category 'class initialization') -----<br>  initializeStructureTypes<br>         "Reset all non-pointer struct types to zero and their pointer companions to the appropriate pointer size."<br>  <br>      StructTypes ifNil: [<br>                  StructTypes := WeakValueDictionary new].<br>      <br>      self cleanupUnusedTypes.<br>      <br>      StructTypes valuesDo:[:structType |<br>           structType "asNonPointerType"<br>                       compiledSpec: (WordArray with: self structureSpec);<br>                   byteAlignment: nil.<br>           structType asPointerType<br>+                     compiledSpec: (WordArray with: self pointerSpec + self structureSpec);<br>-                       compiledSpec: (WordArray with: self pointerSpec);<br>                     byteAlignment: nil].!<br><br>Item was changed:<br>  ----- Method: ExternalType>>handle:at: (in category 'external data') -----<br>  handle: handle at: byteOffset<br>       "Read the receiver's external type using the given handle and the byteOffset. This is the dynamic version of #readFieldAt:."<br>    <br>+     | address value |<br>-    | result |<br>-   self checkType.<br>-      <br>      self isPointerType<br>+           ifTrue:<br>+                      [address := handle pointerAt: byteOffset length: self byteSize.<br>+                      ^ExternalData<br>+                                fromHandle: address<br>+                          type: self asNonPointerType].<br>+        self isAtomic<br>+                ifTrue:<br>+                      ["Answer atomic value"<br>+                     value := handle<br>-              ifFalse: [<br>-                   "Answer atomic value"<br>-                      ^ handle<br>                              perform: (AtomicSelectors at: self atomicType)<br>+                               with: byteOffset.<br>+                    ^referentClass ifNil: [value] ifNotNil: [referentClass fromHandle: value]].<br>+ <br>+      referentClass isNil<br>+          ifTrue: [self error: 'unknown type'].<br>+        self isEmpty ifTrue: [self error: 'Empty structure'].<br>+                <br>+     ^referentClass fromHandle: (handle structAt: byteOffset length: self byteSize)!<br>-                              with: byteOffset]<br>-            ifTrue: [<br>-                    ^ referentClass<br>-                              ifNotNil: [<br>-                                  "Answer structure, union, or type alias"<br>-                                   referentClass fromHandle: (handle pointerAt: byteOffset length: self byteSize)]<br>-                              ifNil: [<br>-                                     "Answer wrapper that points to external data"<br>-                                      result := ExternalData<br>-                                               fromHandle: (handle pointerAt: byteOffset length: self byteSize)<br>-                                             type: self.<br>-                                  self = ExternalType string<br>-                                           ifTrue: [result fromCString]<br>-                                                 ifFalse: [result]]]!<br><br>Item was changed:<br>  ----- Method: ExternalType>>handle:at:put: (in category 'external data') -----<br>  handle: handle at: byteOffset put: value<br>         "Write a value using the receiver's external type at the given handle and byteOffset. This is the dynamic version of #writeFieldAt:with:."<br>  <br>-         self checkType.<br>-      <br>      self isPointerType<br>-           ifFalse: [ "set atomic value"<br>-                      self flag: #addTypeCheck. "mt: Note that there is currently no mapping from objects that represent valid atomics to atomic types."<br>-                         handle<br>-                               perform: ((AtomicSelectors at: self atomicType), 'put:') asSymbol<br>-                            with: byteOffset<br>-                             with: value]<br>                  ifTrue: [ "set pointer to struct/union/alias"<br>+                      self assert: [value externalType == self asNonPointerType].<br>-                  self assert: [value externalType == self].<br>                    handle<br>                                pointerAt: byteOffset<br>                                 put: value getHandle<br>+                                 length: self byteSize.<br>+                       ^value].<br>+             <br>+     self isAtomic<br>+                ifTrue:<br>+                      [ "set atomic value"<br>+                       self flag: #addTypeCheck. "mt: Note that there is currently no mapping from objects that represent valid atomics to atomic types."<br>+                         handle<br>+                               perform: ((AtomicSelectors at: self atomicType), 'put:') asSymbol<br>+                            with: byteOffset<br>+                             with: value.<br>+                         ^value].<br>+                     <br>+     referentClass isNil<br>+          ifTrue: [self error: 'unknown type'].<br>+        self isEmpty ifTrue: [self error: 'Empty structure'].<br>+ <br>+    self assert: [value externalType == self].<br>+   handle structAt: byteOffset put: value getHandle length: self byteSize.<br>+      ^value<br>+       !<br>-                            length: self byteSize].!<br><br><br></div></blockquote></div><br>
</blockquote></div>
</div></blockquote></div><br>
</blockquote></div>
</div></blockquote>
                                        </div></div></blockquote></div><br>
</blockquote></div>
</blockquote></div></div>