VM Maker: VMMaker.oscog-akg.2340.mcz

 
Alistair Grant uploaded a new version of VMMaker to project VM Maker:
http://source.squeak.org/VMMaker/VMMaker.oscog-akg.2340.mcz

==================== Summary ====================

Name: VMMaker.oscog-akg.2340
Author: akg
Time: 3 March 2018, 3:37:32.917843 pm
UUID: 27e21eb5-be74-4672-a779-32c073498b95
Ancestors: VMMaker.oscog-eem.2339

Extend FilePlugin to allow a file to be opened using either the file descriptor (fd) or FILE* in Pharo.

Original PR: pharo-project/pharo-vm#108
Updated PR: pharo-project/pharo-vm#142

(both superseeded)

Many thanks to @zecke (Holger Freyther) for the original code.

As a (redundant) example of how this can be used, to open stderr (fd=2)
for writing:

| stderr |

stderr := BinaryFileStream handle: (FilePluginPrims new
    openFileDescriptor: 2 writable: true)
        file: (File named: 'fd2')
        forWrite: true.

stderr nextPutAll: 'Hello World'; lf.

=============== Diff against VMMaker.oscog-eem.2339 ===============

Item was added:
+ ----- Method: FilePlugin>>cfileRecordSize (in category 'pharo primitives') -----
+ cfileRecordSize
+ "Return the size of a stdio FILE* handle"
+ <inline: #always>
+ ^self sizeof: #'FILE*'!

Item was added:
+ ----- Method: FilePlugin>>fileOpenFd:write: (in category 'private - pharo') -----
+ fileOpenFd: fd write: writeFlag
+ "Open the fd as file. Answer the file oop."
+ | file fileOop |
+ <option: #PharoVM>
+ <var: #file type: 'SQFile *'>
+ <var: 'fd' type: 'int'>
+ <export: true>
+ fileOop := interpreterProxy instantiateClass: interpreterProxy classByteArray indexableSize: self fileRecordSize.
+ file := self fileValueOf: fileOop.
+ interpreterProxy failed
+ ifFalse: [self cCode: 'sqFileFdOpen(file, fd, writeFlag)' inSmalltalk: [file]].
+ ^ fileOop!

Item was added:
+ ----- Method: FilePlugin>>fileOpenFile:write: (in category 'private - pharo') -----
+ fileOpenFile: cfile write: writeFlag
+ "Open the FILE* as file. Answer the file oop."
+ | file fileOop |
+ <option: #PharoVM>
+ <var: #file type: 'SQFile *'>
+ <var: 'cfile' type: 'FILE *'>
+ <export: true>
+ fileOop := interpreterProxy instantiateClass: interpreterProxy classByteArray indexableSize: self fileRecordSize.
+ file := self fileValueOf: fileOop.
+ interpreterProxy failed
+ ifFalse: [self cCode: 'sqFileFileOpen(file, cfile, writeFlag)' inSmalltalk: [file]].
+ ^ fileOop!

Item was added:
+ ----- Method: FilePlugin>>primitiveFileOpenUseFile (in category 'pharo primitives') -----
+ primitiveFileOpenUseFile
+ | writeFlag cfileOop cfile filePointer |
+ <option: #PharoVM>
+ <var: 'cfile' type: 'FILE* '>
+ <export: true>
+ writeFlag := interpreterProxy
+ booleanValueOf: (interpreterProxy stackValue: 0).
+ cfileOop := interpreterProxy stackValue: 1.
+ (((interpreterProxy isBytes: cfileOop) and:
+ [(interpreterProxy byteSizeOf: cfileOop) = self cfileRecordSize]))
+ ifFalse: [^interpreterProxy primitiveFailFor: PrimErrBadArgument].
+ cfile := interpreterProxy firstIndexableField: cfileOop.
+ interpreterProxy failed ifFalse:
+ [filePointer := self fileOpenFile: cfile write: writeFlag].
+ interpreterProxy failed ifFalse:
+ [^interpreterProxy pop: 3 "rcvr, name, writeFlag"
+ thenPush: filePointer].
+ ^interpreterProxy primitiveFail.!

Item was added:
+ ----- Method: FilePlugin>>primitiveFileOpenUseFileDescriptor (in category 'pharo primitives') -----
+ primitiveFileOpenUseFileDescriptor
+ | writeFlag fdPointer fd filePointer |
+ <option: #PharoVM>
+ <var: 'fd' type: 'int'>
+ <export: true>
+ writeFlag := interpreterProxy
+ booleanValueOf: (interpreterProxy stackValue: 0).
+ fdPointer := interpreterProxy stackValue: 1.
+ (interpreterProxy isIntegerObject: fdPointer)
+ ifFalse: [^ interpreterProxy primitiveFailFor: PrimErrBadArgument].
+ fd := interpreterProxy integerValueOf: fdPointer.
+ filePointer := self fileOpenFd: fd write: writeFlag.
+ interpreterProxy failed
+ ifFalse: [^interpreterProxy pop: 3 "rcvr, name, writeFlag"
+ thenPush: filePointer].
+ ^interpreterProxy primitiveFail.!

 
On Sat, 3 Mar 2018, [hidden email] wrote:

Will it not interfere with the VM's existing mechanism to access stdin,
stdout and stderr?
Why is it Pharo only?

Levente

 


> On 3. Mar 2018, at 14:54, Levente Uzonyi <[hidden email]> wrote:
>


>
> Will it not interfere with the VM's existing mechanism to access stdin, stdout and stderr?
> Why is it Pharo only?

Squeak devs opposed this so we are guarding it with PharoVM.

holger

 
On Saturday 03 March 2018 08:30 PM, Holger Freyther wrote:
How about using a feature macro define like FD_NUMERIC for such extensions?

Regards .. Subbu

 
On Sat, 3 Mar 2018, Holger Freyther wrote:

Can you point me to the discussion?

Levente

>
> holger

 
Hi Levente,

On 3 March 2018 at 15:54, Levente Uzonyi <[hidden email]> wrote:
It shouldn't.  It adds two new primitives, which call a couple of
support methods / procedures.  No existing code is changed.

The example I gave wasn't meant to be used as a mechanism to access
stderr, it's just a well known file descriptor.



> Why is it Pharo only?

I'm happy for it to be included with squeak (or any of the other VMs).

Cheers,
Alistair

 


> On 3. Mar 2018, at 15:08, Levente Uzonyi <[hidden email]> wrote:
>

Hey!


>> Squeak devs opposed this so we are guarding it with PharoVM.
>
> Can you point me to the discussion?

Sure. It is my thread from 2016. http://forum.world.st/Adding-new-primitives-to-FilePlugin-td4916711.html

holger

 
On Sat, 3 Mar 2018, Holger Freyther wrote:

Maybe it's just me, but I don't see any opposition in that thread.

Levente

>
> holger

 
On Sat, Mar 03, 2018 at 04:50:40PM +0100, Levente Uzonyi wrote:
For my part, I was recommending a way to proceed, that's all.

Regarding the "#if PharoVM", I don't see a need for this. After all, people
who do not want to use a primitive are free to not use it.

I do think that a working implementation should be provided for Windows.
A Windows HANDLE is roughly equivalent to a Unix file descriptor, and the
*file element of SQFile is a HANDLE, so I expect that the implementation
should be trivial.

Dave
 

 
Hi Holger,

I don't see Squeak opposing anything.  I see a conservative suggestion from David at the beginning followed by encouragement from Tim and I to go ahead and add even more primitives.  Did you feel opposed?

Alistair, please remove the PharoVM guard.   We squeakers would love to use this goodness too.  In fact that reminds me that we should add porting Colin's new file interface from Pharo to Squeak to what should be included in Squeak 6.
>
> holger

 
On Sat, Mar 03, 2018 at 11:43:01AM -0500, David T. Lewis wrote:
I should have looked at the code before replying. The Windows implementation
would be trivial, but it does not make any sense.

Sorry for the noise,
Dave

 
On 3 March 2018 at 18:36, Eliot Miranda <[hidden email]> wrote:
On the way...

https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/223 is checking
that it builds on all platforms correctly.

I'll submit the changes to FilePluginPrims in the image in the next day or so.

Cheers,
Alistair

 
On Sat, Mar 03, 2018 at 12:42:02PM -0500, David T. Lewis wrote:
Is it too late to ask about the names of these primitives? I'm not sure that
I understand the use case, but I am looking at the discussion referenced earlier:
  http://forum.world.st/Adding-new-primitives-to-FilePlugin-td4916711.html

From this I would assume that the intended usage is to get a previously opened
file descriptor or a previously opened *FILE pointer by means of some FFI call,
then pass this to one of the two new primitives in order to build a SQFile data
structure that is returned to the image in the form of a ByteArray. Presumably
the ByteArray would then be used to set the fileID field of a file stream (but
I am not clear on whether that is the intent).

If I am reading the code correctly, that is exactly what the two primitives do.
Is that right?

The names and comments say that the primitives are actually opening the files,
which appears not to be the case.

  primitiveFileOpenUseFile
        "Open the file with the supplied FILE* and writeFlag.
        FILE* must be supplied in a byte object (ByteArray) with the platform address size.
        writeFlag must be a boolean."

  primitiveFileOpenUseFileDescriptor
        "Open the file with the supplied file descriptor and writeFlag.
        fileDescriptor must be an integer.
        writeFlag must be a boolean."

I'm not sure I can think of any good names off the top of my head, but if
the above is right, and given that the ByteArray returned by the primitive
corresponds to the fileID field in a FileStream, then maybe it would be something
like #primitiveFileIDBytesFromFD (for the integer file descriptor or Windows
integer HANDLE) and #primitiveFileIDBytesFromStdioStream (for a *FILE).

It actually does look like the primitive that uses integer file descriptor
would be appropriate for Windows, so that probably can be implemented.
On Windows, you would still need some way to deal with the file handle registry
in FilePlugin, because Andreas was rather strenuously opposed to people (such
as for example me!) doing things in the FilePlugin with file handles that did
not actually originate from the plugin. He was trying to find a more general
solution to this for security reasons, although we never actually implemented
any solution and the handle registry is probably still in effect on Windows.

Please don't take this as objections, I'm just trying to understand the intent
and maybe suggest better naming or comments in the primitives.

Dave

 
On Sat, Mar 03, 2018 at 03:12:10PM -0500, David T. Lewis wrote:
>  
>
> Is it too late to ask about the names of these primitives? I'm not sure that
> I understand the use case, but I am looking at the discussion referenced earlier:
>   http://forum.world.st/Adding-new-primitives-to-FilePlugin-td4916711.html

I don't think it is too late.  They only exist in a pre-release version
of the Pharo VM, and there's no smalltalk code in the image at the
moment to use them.


> From this I would assume that the intended usage is to get a previously opened
> file descriptor or a previously opened *FILE pointer by means of some FFI call,
> then pass this to one of the two new primitives in order to build a SQFile data
> structure that is returned to the image in the form of a ByteArray. Presumably
> the ByteArray would then be used to set the fileID field of a file stream (but
> I am not clear on whether that is the intent).
>
> If I am reading the code correctly, that is exactly what the two primitives do.
> Is that right?

That's my understanding, i.e. to allow smalltalk streams to operate on
files that have been opened by third-party libraries.



> The names and comments say that the primitives are actually opening the files,
> which appears not to be the case.
>
>   primitiveFileOpenUseFile
> "Open the file with the supplied FILE* and writeFlag.
> FILE* must be supplied in a byte object (ByteArray) with the platform address size.
> writeFlag must be a boolean."

Yep (mea culpa, this is my comment).

>   primitiveFileOpenUseFileDescriptor
> "Open the file with the supplied file descriptor and writeFlag.
> fileDescriptor must be an integer.
> writeFlag must be a boolean."

I agree that based on your description above, it isn't really newly
opening a file, however it is calling fdopen(), which does set
a precendent.


> I'm not sure I can think of any good names off the top of my head, but if
> the above is right, and given that the ByteArray returned by the primitive
> corresponds to the fileID field in a FileStream, then maybe it would be something
> like #primitiveFileIDBytesFromFD (for the integer file descriptor or Windows
> integer HANDLE) and #primitiveFileIDBytesFromStdioStream (for a *FILE).

Maybe #primitiveConnectoToFileDescriptor and #primitiveConnectToFile?




Cheers,
Alistair

 
On Sun, Mar 04, 2018 at 11:26:18AM +0000, Alistair Grant wrote:
Yes, I think that fdopen() might be a source of confusion. It is a poor
function name, since is does not open a file or a file descriptor or anything
else, though at least it does rhymeswith "fopen". But I guess it is probably
too late to complain to the guys who wrote Unix ;-)


Those names sound good to me.

Dave