Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GAP.Globals.Display once a GAP error has occured #516

Closed
zickgraf opened this issue Sep 7, 2020 · 2 comments · Fixed by #671
Closed

GAP.Globals.Display once a GAP error has occured #516

zickgraf opened this issue Sep 7, 2020 · 2 comments · Fixed by #671

Comments

@zickgraf
Copy link

zickgraf commented Sep 7, 2020

Consider the following code and output:

               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.5.1 (2020-08-25)
 _/ |\__'_|_|_|\__'_|  |  
|__/                   |

julia> using GAP
 ┌───────┐   GAP 4.11.0 of 29-Feb-2020
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-julia64-kv7-v1.5
 Configuration:  gmp 6.2.0, Julia GC, Julia 1.5.1, readline
 Loading the library and packages ...
 Packages:   GAPDoc 1.dev, PrimGrp 3.4.0, SmallGrp 1.4.1, TransGrp 2.0.5
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'

julia> GAP.Globals.Display( g"test1" )
test1

julia> GAP.Display( g"test2" )
test2

julia> GAP.evalstr( "error" )
ERROR: Error thrown by GAP: Error, Variable: 'error' must have a value

Stacktrace:
 [1] error_handler() at .julia/packages/GAP/tkivN/src/GAP.jl:157
 [2] evalstr_ex at .julia/packages/GAP/tkivN/src/ccalls.jl:36 [inlined]
 [3] evalstr(::String) at .julia/packages/GAP/tkivN/src/ccalls.jl:63
 [4] top-level scope at REPL[4]:1
 [5] run_repl(::REPL.AbstractREPL, ::Any) at /build/julia/src/julia-1.5.1/usr/share/julia/stdlib/v1.5/REPL/src/REPL.jl:288

julia> GAP.Globals.Display( g"test3" )

julia> GAP.Display( g"test4" )
test4

julia>

The string test3 is not printed. In fact, nothing seems to be printed using GAP.Globals.Display after an error has occured. GAP.Display seems to work fine.

A related question we are interested in: Which stream is Display outputting to? We would like to turn the print formatting off for this stream as we did in #513.

@ThomasBreuer
Copy link
Member

@zickgraf Thanks for this report.

I have played around more with this problem (in Julia 1.4.0).
After an error, as shown above, GAP's View, Print, Display do all not print anything to the screen.
However, these functions seem not to work properly also without an error; for example, I get the following.

julia> using GAP
[...]

julia> GAP.Globals.Display( GAP.evalstr( "10^150" ) )
100000000000000000000000000000000000000000000000000000000000000000000000000000\
0000000000000000000000000000000000000000000000000000000000000000000000000

julia> GAP.Globals.Print( GAP.evalstr( "10^150" ) )
100000000000000000000000000000000000000000000000000000000000000000000000000000\

julia> GAP.Globals.View( GAP.evalstr( "10^150" ) )
0000000000000000000000000000000000000000000000000000000000000000000000000
 10000000000000000000000000000000000000000000000000000000000000000000000000000\

julia> GAP.Globals.View( GAP.evalstr( "10^150" ) )
000000000000000000000000000000000000000000000000000000000000000000000000001000\
000000000000000000000000000000000000000000000000000000000000000000000000000000\

julia> GAP.Globals.View( GAP.evalstr( "10^150" ) )
000000000000000000000000000000000000000000000000000000000000000000000100000000\
000000000000000000000000000000000000000000000000000000000000000000000000000000\

julia> GAP.Globals.Display( GAP.evalstr( "10^150" ) )
0000000000000000000000000000000000000000000000000000000000000000
100000000000000000000000000000000000000000000000000000000000000000000000000000\
0000000000000000000000000000000000000000000000000000000000000000000000000

Can you reproduce this behaviour?

Concerning the question to which stream the output of GAP.Globals.Display is sent:
GAP gives the advice (see ?SetPrintFormattingStatus) to call SetPrintFormattingStatus( "*stdout*", false ) in order to switch off the formatting. This works for Print, Display, etc. in GAP, but apparently it does not help when GAP is called from Julia.
Is this a feature or another bug?

@zickgraf
Copy link
Author

zickgraf commented Sep 7, 2020

Can you reproduce this behaviour?

Not exactly, but in general yes. The result seems to depend on the width of the terminal.

Observation: If I Print a string of length shorter than my terminal width, I get no output at first, but the output is shown later once something exceeds the width of my terminal:

julia> GAP.Globals.Print( GAP.evalstr( "1" ) )

julia> GAP.Globals.Print( GAP.evalstr( "2" ) )

julia> GAP.Globals.Print( GAP.evalstr( "3" ) )

julia> GAP.Globals.Print( GAP.evalstr( "10^150" ) )
123
100000000000000000000000000000000000000000000000000000000000000000000000000\
000000000000000000000000000000000000000000000000000000000000000000000000000\

julia>

I assume something buffers the output until some condition (newline?) is reached. If I immediately print a newline, everything works as expected:

julia> GAP.Globals.Print( GAP.evalstr( "\"1\\n\"" ) )
1

julia>

GAP gives the advice (see ?SetPrintFormattingStatus) to call SetPrintFormattingStatus( "stdout", false ) in order to switch off the formatting. This works for Print, Display, etc. in GAP, but apparently it does not help when GAP is called from Julia.
Is this a feature or another bug?

I can confirm that it does not work from Julia, not even in GAP.prompt(). I assume there is some more magic happening in the background.

fingolfin added a commit to fingolfin/GAP.jl that referenced this issue Jul 23, 2021
Previously, we threw a Julia exception in the `error_handler` function,
which is called by GAP's `JUMP_TO_CATCH`, which in turn is invoked by
GAP's `ErrorInner`.

The problem with that is that by throwing a Julia exception, we aborted the
GAP exception handling prematurely. As a result, any surrounding `GAP_CATCH`
blocks had no chance to run. This had primarily two noticeable effects:

1. Input/output redirection set up by surrounding GAP code via `OpenInput`,
   `OpenInputStream`, `OpenOutput`, `OpenOutputStream` was not reverted.

2. The pointer GAP uses to keep track of which code is currently being
   executed ("LVars") was not reset.

Effect 2 could be counteracted by calling `SWITCH_TO_BOTTOM_LVARS`.

Effect 1 in past GAP versions mostly lead to some output which was not showing
up, see <oscar-system#516>. With the current
GAP kernel code, though, the "stack of outputs" was changed into a "linked
list of outputs", with most outputs residing on the execution stack. As a
result, failing to close outputs now can lead to crashes or general weirdness.

To fix this, we need to let GAP follow its sequence of exception handlers, and
only throw a Julia exception at the very end. The standard way to do this is
to surround any calls into the GAP kernel by `GAP_TRY` and `GAP_CATCH`. But
the way these C macros work would mean that we couldn't simply use `ccall`
anymore. We'd have to add tons of wrapper functions. Plus, we'd loose
performance even in the common case were no exception is generated.

So instead, I modified the GAP kernel to track how many nested `GAP_TRY` there
are, and pass this value to the callback already present in `GAP_THROW`. We
now install such a callback in `GAP.jl` in addition to the `JUMP_TO_CATCH`
callback: the latter now only records the error message in a global variable,
and then returns (i.e., it doesn't throw an exception anymore). The
`ThrowObserver` callback then throws the actual exception using this error
messages. This is done in PR <gap-system/gap#4617>.

All this together almost fixes "everything", except some GAP kernel code
failed to use `GAP_TRY` / `GAP_CATCH` and thus sometimes failed to clean
up its input/output stream overrides, even with our tricks. This is fixed
in PR <gap-system/gap#4616>.

Of course to test all this, I also had to write scripts which makes it
"easy enough" to test a custom GAP build inside of GAP.jl; the result is
in PR <oscar-system#667>.

The final patch here is very small in the end; the difficult part was getting
there. Oh, and lots of time waiting for configure scripts, C compilers, Julia
precompatilation and more, cf. <https://xkcd.com/303/>.
fingolfin added a commit to fingolfin/GAP.jl that referenced this issue Jul 26, 2021
Previously, we threw a Julia exception in the `error_handler` function,
which is called by GAP's `JUMP_TO_CATCH`, which in turn is invoked by
GAP's `ErrorInner`.

The problem with that is that by throwing a Julia exception, we aborted the
GAP exception handling prematurely. As a result, any surrounding `GAP_CATCH`
blocks had no chance to run. This had primarily two noticeable effects:

1. Input/output redirection set up by surrounding GAP code via `OpenInput`,
   `OpenInputStream`, `OpenOutput`, `OpenOutputStream` was not reverted.

2. The pointer GAP uses to keep track of which code is currently being
   executed ("LVars") was not reset.

Effect 2 could be counteracted by calling `SWITCH_TO_BOTTOM_LVARS`.

Effect 1 in past GAP versions mostly lead to some output which was not showing
up, see <oscar-system#516>. With the current
GAP kernel code, though, the "stack of outputs" was changed into a "linked
list of outputs", with most outputs residing on the execution stack. As a
result, failing to close outputs now can lead to crashes or general weirdness.

To fix this, we need to let GAP follow its sequence of exception handlers, and
only throw a Julia exception at the very end. The standard way to do this is
to surround any calls into the GAP kernel by `GAP_TRY` and `GAP_CATCH`. But
the way these C macros work would mean that we couldn't simply use `ccall`
anymore. We'd have to add tons of wrapper functions. Plus, we'd loose
performance even in the common case were no exception is generated.

So instead, I modified the GAP kernel to track how many nested `GAP_TRY` there
are, and pass this value to the callback already present in `GAP_THROW`. We
now install such a callback in `GAP.jl` in addition to the `JUMP_TO_CATCH`
callback: the latter now only records the error message in a global variable,
and then returns (i.e., it doesn't throw an exception anymore). The
`ThrowObserver` callback then throws the actual exception using this error
messages. This is done in PR <gap-system/gap#4617>.

All this together almost fixes "everything", except some GAP kernel code
failed to use `GAP_TRY` / `GAP_CATCH` and thus sometimes failed to clean
up its input/output stream overrides, even with our tricks. This is fixed
in PR <gap-system/gap#4616>.

Of course to test all this, I also had to write scripts which makes it
"easy enough" to test a custom GAP build inside of GAP.jl; the result is
in PR <oscar-system#667>.

The final patch here is very small in the end; the difficult part was getting
there. Oh, and lots of time waiting for configure scripts, C compilers, Julia
precompatilation and more, cf. <https://xkcd.com/303/>.
fingolfin added a commit to fingolfin/GAP.jl that referenced this issue Jul 26, 2021
Previously, we threw a Julia exception in the `error_handler` function,
which is called by GAP's `JUMP_TO_CATCH`, which in turn is invoked by
GAP's `ErrorInner`.

The problem with that is that by throwing a Julia exception, we aborted the
GAP exception handling prematurely. As a result, any surrounding `GAP_CATCH`
blocks had no chance to run. This had primarily two noticeable effects:

1. Input/output redirection set up by surrounding GAP code via `OpenInput`,
   `OpenInputStream`, `OpenOutput`, `OpenOutputStream` was not reverted.

2. The pointer GAP uses to keep track of which code is currently being
   executed ("LVars") was not reset.

Effect 2 could be counteracted by calling `SWITCH_TO_BOTTOM_LVARS`.

Effect 1 in past GAP versions mostly lead to some output which was not showing
up, see <oscar-system#516>. With the current
GAP kernel code, though, the "stack of outputs" was changed into a "linked
list of outputs", with most outputs residing on the execution stack. As a
result, failing to close outputs now can lead to crashes or general weirdness.

To fix this, we need to let GAP follow its sequence of exception handlers, and
only throw a Julia exception at the very end. The standard way to do this is
to surround any calls into the GAP kernel by `GAP_TRY` and `GAP_CATCH`. But
the way these C macros work would mean that we couldn't simply use `ccall`
anymore. We'd have to add tons of wrapper functions. Plus, we'd loose
performance even in the common case were no exception is generated.

So instead, I modified the GAP kernel to track how many nested `GAP_TRY` there
are, and pass this value to the callback already present in `GAP_THROW`. We
now install such a callback in `GAP.jl` in addition to the `JUMP_TO_CATCH`
callback: the latter now only records the error message in a global variable,
and then returns (i.e., it doesn't throw an exception anymore). The
`ThrowObserver` callback then throws the actual exception using this error
messages. This is done in PR <gap-system/gap#4617>.

All this together almost fixes "everything", except some GAP kernel code
failed to use `GAP_TRY` / `GAP_CATCH` and thus sometimes failed to clean
up its input/output stream overrides, even with our tricks. This is fixed
in PR <gap-system/gap#4616>.

Of course to test all this, I also had to write scripts which makes it
"easy enough" to test a custom GAP build inside of GAP.jl; the result is
in PR <oscar-system#667>.

The final patch here is very small in the end; the difficult part was getting
there. Oh, and lots of time waiting for configure scripts, C compilers, Julia
precompatilation and more, cf. <https://xkcd.com/303/>.
fingolfin added a commit to fingolfin/GAP.jl that referenced this issue Jul 26, 2021
Previously, we threw a Julia exception in the `error_handler` function,
which is called by GAP's `JUMP_TO_CATCH`, which in turn is invoked by
GAP's `ErrorInner`.

The problem with that is that by throwing a Julia exception, we aborted the
GAP exception handling prematurely. As a result, any surrounding `GAP_CATCH`
blocks had no chance to run. This had primarily two noticeable effects:

1. Input/output redirection set up by surrounding GAP code via `OpenInput`,
   `OpenInputStream`, `OpenOutput`, `OpenOutputStream` was not reverted.

2. The pointer GAP uses to keep track of which code is currently being
   executed ("LVars") was not reset.

Effect 2 could be counteracted by calling `SWITCH_TO_BOTTOM_LVARS`.

Effect 1 in past GAP versions mostly lead to some output which was not showing
up, see <oscar-system#516>. With the current
GAP kernel code, though, the "stack of outputs" was changed into a "linked
list of outputs", with most outputs residing on the execution stack. As a
result, failing to close outputs now can lead to crashes or general weirdness.

To fix this, we need to let GAP follow its sequence of exception handlers, and
only throw a Julia exception at the very end. The standard way to do this is
to surround any calls into the GAP kernel by `GAP_TRY` and `GAP_CATCH`. But
the way these C macros work would mean that we couldn't simply use `ccall`
anymore. We'd have to add tons of wrapper functions. Plus, we'd loose
performance even in the common case were no exception is generated.

So instead, I modified the GAP kernel to track how many nested `GAP_TRY` there
are, and pass this value to the callback already present in `GAP_THROW`. We
now install such a callback in `GAP.jl` in addition to the `JUMP_TO_CATCH`
callback: the latter now only records the error message in a global variable,
and then returns (i.e., it doesn't throw an exception anymore). The
`ThrowObserver` callback then throws the actual exception using this error
messages. This is done in PR <gap-system/gap#4617>.

All this together almost fixes "everything", except some GAP kernel code
failed to use `GAP_TRY` / `GAP_CATCH` and thus sometimes failed to clean
up its input/output stream overrides, even with our tricks. This is fixed
in PR <gap-system/gap#4616>.

Of course to test all this, I also had to write scripts which makes it
"easy enough" to test a custom GAP build inside of GAP.jl; the result is
in PR <oscar-system#667>.

The final patch here is very small in the end; the difficult part was getting
there. Oh, and lots of time waiting for configure scripts, C compilers, Julia
precompatilation and more, cf. <https://xkcd.com/303/>.
@fingolfin fingolfin linked a pull request Jul 27, 2021 that will close this issue
fingolfin added a commit that referenced this issue Jul 27, 2021
Previously, we threw a Julia exception in the `error_handler` function,
which is called by GAP's `JUMP_TO_CATCH`, which in turn is invoked by
GAP's `ErrorInner`.

The problem with that is that by throwing a Julia exception, we aborted the
GAP exception handling prematurely. As a result, any surrounding `GAP_CATCH`
blocks had no chance to run. This had primarily two noticeable effects:

1. Input/output redirection set up by surrounding GAP code via `OpenInput`,
   `OpenInputStream`, `OpenOutput`, `OpenOutputStream` was not reverted.

2. The pointer GAP uses to keep track of which code is currently being
   executed ("LVars") was not reset.

Effect 2 could be counteracted by calling `SWITCH_TO_BOTTOM_LVARS`.

Effect 1 in past GAP versions mostly lead to some output which was not showing
up, see <#516>. With the current
GAP kernel code, though, the "stack of outputs" was changed into a "linked
list of outputs", with most outputs residing on the execution stack. As a
result, failing to close outputs now can lead to crashes or general weirdness.

To fix this, we need to let GAP follow its sequence of exception handlers, and
only throw a Julia exception at the very end. The standard way to do this is
to surround any calls into the GAP kernel by `GAP_TRY` and `GAP_CATCH`. But
the way these C macros work would mean that we couldn't simply use `ccall`
anymore. We'd have to add tons of wrapper functions. Plus, we'd loose
performance even in the common case were no exception is generated.

So instead, I modified the GAP kernel to track how many nested `GAP_TRY` there
are, and pass this value to the callback already present in `GAP_THROW`. We
now install such a callback in `GAP.jl` in addition to the `JUMP_TO_CATCH`
callback: the latter now only records the error message in a global variable,
and then returns (i.e., it doesn't throw an exception anymore). The
`ThrowObserver` callback then throws the actual exception using this error
messages. This is done in PR <gap-system/gap#4617>.

All this together almost fixes "everything", except some GAP kernel code
failed to use `GAP_TRY` / `GAP_CATCH` and thus sometimes failed to clean
up its input/output stream overrides, even with our tricks. This is fixed
in PR <gap-system/gap#4616>.

Of course to test all this, I also had to write scripts which makes it
"easy enough" to test a custom GAP build inside of GAP.jl; the result is
in PR <#667>.

The final patch here is very small in the end; the difficult part was getting
there. Oh, and lots of time waiting for configure scripts, C compilers, Julia
precompatilation and more, cf. <https://xkcd.com/303/>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants