UpEnv and Where are broken

[ChrisJefferson]

I tried doing the example of DownEnv/UpEnv from section 6.5 of the manual, and found 2 issues:

1. If you perform DownEnv, the output of Where doesn't change (the documentation suggests the backtrace should move to wherever we are now in the environment).

2. UpEnv(n) always moves all the way up to the top, regardless of what value of n is passed.

I hope to get around to looking at these, but if someone else does first, great. They seem to have been broken since at least 4.7.0.

[stevelinton]

I think UpEnv works, but Where doesn't, so it looks like it hasn't.

The problem seems to be the kernel and GAP level variables ErrorLVars getting out of sync.
Where uses the GAP one and DownEncv and UpEnv adjust the kernel one.

On entry to a break loop they both get set to the CurrentLVars (stack frame) but there doesn't seem to be anything to keep them in sync.

The "right" solution, I think, is to move DownEnv and UpEnv to the library and get rid of the kernel version. There is an API which probably exposes enough of the stack frames to do this. Some care is needed to make sure that variable lookup in the break loop works as expect though (for at least some expectations).

I'll look into it.

[fingolfin]

This is a really annoying bug -- I never reported it, because I thought it was just meant that way, but looking at the manual, it is clear that this is supposed to work better.

The current setup makes debugging really frustrating. Any improvement here would be highly welcome by me.

BTW, I'd actually prefer if the output of Where did not "shift", as suggested by the manual, but rather would track the current position in the backtrace. So, instead of this (taken from the manual)

brk> Where();
 called from
test( n + 1 ); called from
test( n + 1 ); called from
test( n + 1 ); called from
<function>( <arguments> ) called from read-eval-loop
brk> DownEnv();
brk> Where();
 called from
test( n + 1 ); called from
test( n + 1 ); called from
<function>( <arguments> ) called from read-eval-loop
brk>

could we perhaps have something like this?

brk> Where();
 called from
#0* test( n + 1 ); called from
#1  test( n + 1 ); called from
#2  test( n + 1 ); called from
#3  <function>( <arguments> ) called from read-eval-loop
brk> DownEnv();
brk> Where();
 called from
#0  test( n + 1 ); called from
#1* test( n + 1 ); called from
#2  test( n + 1 ); called from
#3  <function>( <arguments> ) called from read-eval-loop
brk>

Here, each level has a number; we always see all levels; and the "active" level is marked with an asterisk.

The one drawback this might have is with very deep backtraces, where it might be better to cut off the backtrace. If that is really a concern, I'd still think showing the levels would be helpful. Also, perhaps we could have a heuristic: If there are fewer than, say, 10 levels, always show all, with a marker. If there are more, then abbreviate it with some heuristic, perhaps like this:

brk> Where();
 called from
#0  test( n + 1 ); called from
#1  test( n + 1 ); called from
   ... skipping ...
#33  test( n + 1 ); called from
#34* test( n + 1 ); called from
#36  test( n + 1 ); called from
#35  <function>( <arguments> ) called from read-eval-loop
brk>

[stevelinton]

Agreed. I got someway through digging into it, but I need to look again. The code (which I wrote) is rather ugly.

[fingolfin]

I have looked into this, and in the course made some cleanups and did some refactoring, see PR #934.

As to UpEnv(): It definitely is broken, and this is not a matter of ErrorLVars in kernel and in the library getting out of sync. Rather, it is caused by ErrorLVars0 (added by @stevelinton on 2012-11-13 21:13:58) not doing its job. Or rather, it does it original job (which was to fix a crash), but does not do what else it was meant to do (namely to keep track of the "original" value of ErrorLVars. This is so because it keeps getting reset by ReadEvalCommand. So once we are in DownEnvInner, the two variables ErrorLVars and ErrorLVars0 effectively are equal.

Moreover, I noticed another problem: ErrorLLevel never ever gets reset. So, if you are in a nested break loop, it may be at a big bogus value. Alas, it doesn't matter right now, because it is only need for UpEnv, which is indeed thoroughly broken.

I need to stop working on this for now, as I have other things to attend to (like preparing my lecture tomorrow ...).

But one last thing: I also have a mostly working patch which impements a nicer backtrace, as displayed above. The only caveat is that the information on level #0 (i.e. the place where the error originated) very quickly gets corrupted: Just enter an unassigned variable name; this triggers a syntax "error", which in turn causes the kernel to invoke SET_BRK_CALL_TO on the current Lvars -- which are the ErrorLVars. This the overwrites (corrupts) the information about the location of the error. I guess this is why Where() currently does not print information about that level, which I think is bad.
So far, the best solution for fixing this problem I had was to modify ErrorInner to immediately extract that information and store it somewhere (and then either restore it whenever needed, and/or use it directly in Where). Another idea (perhaps more elegant) would be to put another (mostly empty) LVars atop the ErrorLVars at least while on the break loop, to avoid this corruption. Of course we then need to be careful to not get confused by that for other operations...

[fingolfin]

(and I feel I don't know enough just now about how LVars are handled, so i won't try to attempt that last hypothetical solution; in particular, it might be totally bonkers to start with -- I wouldn't know... yet... :-)

[ChrisJefferson]

@fingolfin , thanks for looking into this. While you are there, I was trying to use CALL_WITH_CATCH, and either I don't understand how to use it, or it doesn't work. I imagine it's related, so maybe you can look while you are there.

I've also never understood the relationship between SET_BRK_CURR_STAT and SET_BRK_CALL_TO, and why we set one in some places, and the other in others...

[fingolfin]

@ChrisJefferson What exactly is your problem with CALL_WITH_CATCH? It works fine for me, see e.g. https://github.com/fingolfin/UnitTests/blob/master/lib/units.gi

Anyway, if you suspect there is a problem, I think it'd be best to open a separate issue.

And I also was and am confused by SET_BRK_CURR_STAT versus SET_BRK_CALL_TO. Perhaps I can look into it on the weekend or next week... But don't hold your breath on that... ;-)