Smalltalk › Squeak › Squeak VM

[OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

56 messages Options

123

David T Lewis

[OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

Related to pharo-project/pharo#694

You can view, comment on, or merge this pull request online at:

https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/254

Commit Summary

First Version
Improve Win API compatibility
Use the primitive in the main function
code ordering
move function to Win32Main + add stubs for other plateforms for the primitive implementation
remove useless imports

File Changes

Patch Links:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

@VincentBlondeau pushed 1 commit.

e76c9f5 Primitive Returns a bool instead of a int

—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

Hi Vincent, in isStdioDescriptorATTY at line 898 there is no guard to check if pGetFileInformationByHandleEx has already been fetched. The code needs to be something like

static pfnGetFileInformationByHandleEx pGetFileInformationByHandleEx = NULL;

if (! pGetFileInformationByHandleEx) {
	pGetFileInformationByHandleEx = (pfnGetFileInformationByHandleEx)
		GetProcAddress(GetModuleHandle(TEXT("kernel32.dll")),
					"GetFileInformationByHandleEx");
	if (pGetFileInformationByHandleEx == NULL)  return -2;
}

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

Also, in platforms/Cross/plugins/FilePlugin/sqFilePluginBasicPrims.c, the comment "//There is always an TTY to write into for Unix and Mac" (and hence the implementation) is wrong. It should be something like

sqInt sqStdioDescriptorIsATTY(void) {
return isatty(fileno(stdin));
}

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

Thanks Eliot for the piece of advice! I am not an expert in C yet ;)

I made the changes but I was wondering why it is needed to set a guard: the line before just set the pGetFileInformationByHandleEx to NULL.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

@VincentBlondeau pushed 1 commit.

a3cf816 Add Guard for pGetFileInformationByHandleEx + clean real estate + bug

—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

Hi Vincent,

Just based on a quick visual inspection of the code:

It looks like on Unix sqStdioDescriptorIsATTY() checks stdin, while on Windows it checks stdout, which is a bit inconsistent and they can be redirected independently anyway. Why not pass an argument allowing the user to specify which file to check (0, 1, 2)?
We'll want to modify sqFileStdioHandlesInto() in sqWin32FilePrims.c to use the new check (instead of GetConsoleMode()).
Actually, since the result is always stored in the SQFile structure for the stdio streams, the primitive could just return the cached value rather than determining it every time.
sqFileAtEnd() on Windows just assumes that stdin is never at EOF if it is a terminal. I don't know how EOF is signalled in a terminal on Windows (Ctrl-D?), but we could also improve this (although maybe this can be a separate PR).

Thanks!
Alistair

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

timrowledge

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

> On 23-04-2018, at 10:34 PM, akgrant43 <[hidden email]> wrote:
>
> • Actually, since the result is always stored in the SQFile structure for the stdio streams, the primitive could just return the cached value rather than determining it every time.

Unless it can change from external commands?

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Florida: more red in the face

alistairgrant

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

Hi Tim,

On 24 April 2018 at 18:25, tim Rowledge <[hidden email]> wrote:
>
>
>
>> On 23-04-2018, at 10:34 PM, akgrant43 <[hidden email]> wrote:
>>
>> • Actually, since the result is always stored in the SQFile structure for the stdio streams, the primitive could just return the cached value rather than determining it every time.
>
> Unless it can change from external commands?

I'm not aware of any way this can be done, but also don't claim any
particular expertise (for a long time the VM has assumed that it
doesn't change).

If there is a way to change the stdio stream the correct approach is
to ensure that the SQFile structure is updated appropriately, so it
will still be OK to answer the cached value.

Cheers,
Alistair

timrowledge

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

> On 24-04-2018, at 9:34 AM, Alistair Grant <[hidden email]> wrote:
>
>
> Hi Tim,
>
> On 24 April 2018 at 18:25, tim Rowledge <[hidden email]> wrote:
>>
>>
>>
>>> On 23-04-2018, at 10:34 PM, akgrant43 <[hidden email]> wrote:
>>>
>>> • Actually, since the result is always stored in the SQFile structure for the stdio streams, the primitive could just return the cached value rather than determining it every time.
>>
>> Unless it can change from external commands?
>
> I'm not aware of any way this can be done, but also don't claim any
> particular expertise (for a long time the VM has assumed that it
> doesn't change).

Likewise with the not knowing. I tend to the paranoid when it comes to outside resources. Can a file descriptor/stream be changed ? Betting against it being possible on any of the platforms we use is probably a poor idea to be honest.

>
> If there is a way to change the stdio stream the correct approach is
> to ensure that the SQFile structure is updated appropriately, so it
> will still be OK to answer the cached value.

If it were possible to change the status from outside the image and there were some sort of notifier from the OS we'd be ok. Otherwise it's often smart to treat it as a volatile variable. It's like testing that a file exists and is writable and relying upon that staying true over a long period- usually ok but definitely not safe.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Implementation is the sincerest form of flattery.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

@VincentBlondeau pushed 2 commits.

cf60cec Add fd as parameter for sqStdioDescriptorIsATTY
88546ed get rid of getConsoleMode in sqFileStdioHandlesInto + fix bug on powershell

—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

Closed #254.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

Reopened #254.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

Hi Vincent, shouldn't all the
files[0].isStdioStream = sqIsFileDescriptorATTY(0);
occurrences in platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c: sqFileStdioHandlesInto read either
files[0].isStdioStream = sqIsFileDescriptorATTY(STD_INPUT_HANDLE);
or
files[0].isStdioStream = sqIsFileDescriptorATTY(files[0].file);
? Since sqIsFileDescriptorATTY could a HANDLE right? And then you could get rid of the
h = (HANDLE)_get_osfhandle(fdNum);
call in sqIsFileDescriptorATTY

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

The exposed primitive sqIsFileDescriptorATTY should not receive an HANDLE because one needs to call sqIsFileDescriptorATTY to know if an handle is available. But I can change sqWin32FilePrims.c:isFileDescriptorATTY to take an HANDLE as argument instead of a String

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

@VincentBlondeau pushed 1 commit.

7ce2e5e Change isFileDescriptorATTY(int fdNum) to isFileDescriptorATTY(HANDLE fdHandle) (and the other functions that goes with it)

—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

@VincentBlondeau pushed 1 commit.

702a540 rename isFileDescriptorATTY to isFileHandleATTY

—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

Hi Vincent,

After checking out and building VincentBlondeau:addStdoutIsConsolePrimitive I get:

~/pharo7/pharo-snap/pharo-vm/opensmalltalk-vm/build.win32x86/pharo.cog.spur/builddbg/vm
$ gdb PharoConsole.exe
GNU gdb (GDB) (Cygwin 7.10.1-1) 7.10.1
Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-cygwin".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from PharoConsole.exe...done.
(gdb) run --version
Starting program: /home/alistair/pharo7/pharo-snap/pharo-vm/opensmalltalk-vm/build.win32x86/pharo.cog.spur/builddbg/vm/PharoConsole.exe --                                               version
[New Thread 6252.0x16b0]
[New Thread 6252.0x1114]
[New Thread 6252.0x1a68]
[New Thread 6252.0x1d10]

Program received signal SIGSEGV, Segmentation fault.
0x76e1299b in msvcrt!_fileno () from /cygdrive/c/WINDOWS/System32/msvcrt.dll
(gdb) bt
#0  0x76e1299b in msvcrt!_fileno () from /cygdrive/c/WINDOWS/System32/msvcrt.dll
#1  0x00b4b17b in ?? ()
#2  0x00b4b333 in ?? ()
#3  0x00b4c567 in ?? ()
#4  0x00c22a7d in ?? ()
#5  0x00a913e3 in ?? ()
#6  0x74fc9564 in KERNEL32!BaseThreadInitThunk () from /cygdrive/c/WINDOWS/System32/KERNEL32.DLL
#7  0x773529ac in ntdll!EtwProcessPrivateLoggerRequest () from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
#8  0x77352980 in ntdll!EtwProcessPrivateLoggerRequest () from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
#9  0x00000000 in ?? ()
(gdb)

Sorry, but I'm out of time for today. Hopefully I'll get to investigate a bit more over the coming days.

Cheers,
Alistair

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

@VincentBlondeau pushed 1 commit.

f9e79db Use the right functions to deal with WINAPI HANDLEs + refactoring

—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Add accurate check for console stdio + add associated primitive (#254)

In reply to this post by David T Lewis

I was using the wrong function to convert the handle to a number. I found another solution that works, now, I hope it will be working

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

123