Overhaul tracking of current statement, fixing several bugs where the break loop error message referenced the wrong statement #2520

fingolfin · 2018-06-06T14:57:38Z

This PR unifies the code we use to track the current statement and expression. It resolves various bugs where backtraces indicated the wrong statement as the current one, ans also gets rid of some cases where "" was printed.

This needs more work on the long run (in future PRs), in particular for the case where there is an error in the condition expression of an if statement or a loop: then what should we print? Just the expression in which the error occurred? But that could be a simple integer (in if 1 then ... fi), which is not helpful (and in this particular case, would even lead to a "corrupt statement" message for technical reasons).

Or perhaps the whole statement should be printed? But that could span dozens of lines. Slightly better is to print, say, a while loop as while COND do ... od;, i.e. replace the body by .... But (a) the condition still could be multiple lines long, and (b) this does not work very well for an if/elif/elif/.../else/fi with multiple branches.

Or "just the line on which the error occurred" -- but that then depends on the on-disk representation of the data, which opens its own can of worms, so I'd rather avoid it.

For now, I think my favorite would be to just print while COND do (no ... or od;), resp. if COND then or elif COND then etc. But we lack the facilities for that -- esp. for the elif we currently don't even have place where to store on which of the various "branches" of a "multi-condition if statement" we are.

So, this needs work, but not today... I plan to turn the above into an issue later.

Resolves #616
Fixes #2656

fingolfin · 2018-06-06T20:34:41Z

tst/test-error/debug-var.g.out

@@ -109,7 +109,7 @@ brk_2> IsBound(y);
 true
 brk_2> unbound_higher;
 Error, Variable: <debug-variable-64-4> must have a value in
-  <corrupted statement>  called from 
+  Error( "foobar" ); at *stdin*:20 called from 


This is a clear improvement, I'd say

fingolfin · 2018-06-06T20:35:18Z

tst/test-error/method-not-found.g.out

@@ -2,7 +2,7 @@ gap> # test returning from a 'method not found' error
 gap> f:=a->a+a;; f(());
 Error, no method found! For debugging hints type ?Recovery from NoMethodFound
 Error, no 1st choice method found for `+' on 2 arguments at GAPROOT/lib/methsel2.g:250 called from
-a + a at *stdin*:3 called from
+return a + a; at *stdin*:3 called from


Hm.... Is this new behaviour OK? Or do we want the old back? Opinions, anybody?

codecov · 2018-06-06T22:28:38Z

Codecov Report

Merging #2520 into master will increase coverage by 0.7%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #2520     +/-   ##
=========================================
+ Coverage   83.06%   83.76%   +0.7%     
=========================================
  Files         680      681      +1     
  Lines      346665   346339    -326     
=========================================
+ Hits       287941   290105   +2164     
+ Misses      58724    56234   -2490

Impacted Files	Coverage Δ
src/vars.c	`92.7% <ø> (+0.18%)`	⬆️
src/hpc/thread.c	`56.96% <ø> (+0.1%)`	⬆️
src/c_type1.c	`85.07% <ø> (+0.61%)`	⬆️
src/c_oper1.c	`93.44% <ø> (+0.26%)`	⬆️
src/gapstate.h	`71.42% <ø> (ø)`	⬆️
src/hpc/c_type1.c	`85.34% <ø> (+0.25%)`	⬆️
src/hpc/c_oper1.c	`92.64% <ø> (-0.11%)`	⬇️
src/vars.h	`100% <100%> (ø)`	⬆️
src/stats.c	`95.35% <100%> (+0.08%)`	⬆️
src/compiler.c	`88.33% <100%> (-0.04%)`	⬇️
... and 164 more

fingolfin · 2018-09-27T20:01:57Z

There is still much more that should be done on this subject, but I think instead of letting this rot, we should just merge it. To this end, I cleaned up the PR and rebased it again.

fingolfin · 2018-09-27T20:02:49Z

src/error.c

@@ -253,7 +254,7 @@ Obj FuncCALL_WITH_CATCH(Obj self, Obj func, volatile Obj args)
        CHANGED_BAG(res);
        STATE(ThrownObject) = 0;
        SWITCH_TO_OLD_LVARS(currLVars);
-        STATE(CurrStat) = currStat;
+        GAP_ASSERT(currStat == BRK_CALL_TO());


This is a paranoia check, to verify I did not break anything fundamental in the refactoring. It could/should likely be removed eventually.

I personally don't think you can have too many asserting paranoia checks :)

fingolfin · 2018-09-27T20:08:47Z

src/stats.c

+    // HACK: point current "statement" at the condition expression, so
+    // that if there is an error in it, it resp. its location is printed,
+    // not the whole loop statement.
+    SET_BRK_CALL_TO(cond);


We might want to add a few more of these "hacks" for now. The purpose of these hacks is as follows: Without them, you might see:

Error, Rational operations: <divisor> must not be zero in if true = 1 / 0 then return 1; fi; at *stdin*:9 called from

with the hacks, you get:

Error, Rational operations: <divisor> must not be zero in 1 / 0 at *stdin*:9 called from

I.e. it's the difference between printing the condition expression of a loop or if clause, vs. printing the whole loop resp. if statement. Now, a better approach probably would be to either print the actual line(s) containing the expression (as e.g. GDB would do it); or else add a way to print just the "head" of a loop or if (resp.: print the loop, but with the body replaced by, say ...). The former can be difficult because a single expression can of course span many lines, including comments; also, the source file may be modified, leading to output that's not right (just as can happen with gdb); if we wanted to avoid that, we'd have to store the whole file in memory.

The second approach seems much simpler to me, and shouldn't be too hard to implement... (famous last words)

BTW, these HACKS are also limited in so far as they break down if cond is an "immediate" expression, i.e., an INTEXPR or a REFLVAR

I'll look into implementing the second approach now

fingolfin · 2018-09-27T21:01:17Z

tst/testspecial/backtrace.g.out

+gap> f:=function() if 1 < 0 then return 1; elif 1 then return 2; fi; return 3; end;;
+gap> f();
+Error, <expr> must be 'true' or 'false' (not a integer) in
+  1 < 0 at *stdin*:18 called from 


This is wrong, and right now I am not quite sure why it is...

But what is the right thing to print? In a previous comment, I suggested that for loops and if clauses, one could just hide the "bodies", and so e.g. print while 1 do ... od; instead of the full loop. But for if/elif/fi clauses with multiple conditions, this still doesn't seem quite good enough: ideally, we only want to print the if resp. elif with the expression that was executed. But there is no separate statement for that -- we are "in the middle" of a single statement here! I don't see a simple solution for that right now, so I opted to drop both approaches I tried from this PR right now, to be revisited in a future PR. For now, we are at least not doing worse than we were before.

fingolfin · 2018-10-19T09:35:58Z

I have rebased this now again, to resolve conflicts. Would be nice if this could be either merged, or rejected, so that I can move on. My guess is that nobody is looking at it because it is has fallen of the first page of PRs. But if @ChrisJefferson or @stevelinton (who wrote a lot of the affected code) could just have a look at it...

This way, we always know which statement was executed last. The price we pay is a small performance overhead.

Instead of storing the current statement both in `STATE(CurrStat)` and in `STAT_LVARS_PTR(STATE(PtrLVars))`, store them only in the latter location. That allows us to get rid of code for copying between these two locations, too. This means `SET_BRK_CURR_STAT` etc. can be removed, but to keep the patch small, we leave most of them in for now, and just #define them to do nothing. In a few cases, code is changed to use `SET_BRK_CALL_TO` instead.

This reverts commit 70ee840.

In a few select spaces, we insert `SET_BRK_CALL_TO` calls instead, in order to improve certain backtraces.

After evaluating the body of a loop resp. an if-statmeent, and before evaluating the next condition expression, reset the current statement to point to the loop resp. if-statement again.

stevelinton

Having reviewed this PR with @fingolfin and @ChrisJefferson I am happy to approve it.

fingolfin added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) topic: kernel do not review PRs which are not yet ready for a proper external review (e.g. only submitted for test results) labels Jun 6, 2018

fingolfin commented Jun 6, 2018

View reviewed changes

fingolfin force-pushed the mh/backtrace branch from 317ac5f to 2ceff9f Compare June 6, 2018 22:27

fingolfin force-pushed the mh/backtrace branch from 2ceff9f to 15aa7c9 Compare June 6, 2018 22:34

olexandr-konovalov added the gapdays2018-spring Issues and PRs that arose at http://www.gapdays.de/gap-jupyter-days2018 label Jun 8, 2018

fingolfin mentioned this pull request Jun 11, 2018

kernel: don't include debug.h everywhere; and more #2536

Merged

fingolfin force-pushed the mh/backtrace branch 2 times, most recently from ddea702 to a022172 Compare June 13, 2018 16:06

fingolfin force-pushed the mh/backtrace branch from a022172 to 49ac00f Compare July 6, 2018 12:43

stevelinton mentioned this pull request Jul 23, 2018

Error message <corrupted statement> when using [i,j] with invalid indices #2656

Closed

fingolfin force-pushed the mh/backtrace branch 2 times, most recently from 7653a7d to 369a079 Compare August 15, 2018 13:38

fingolfin force-pushed the mh/backtrace branch 2 times, most recently from 331c98e to 6340070 Compare September 11, 2018 12:36

fingolfin force-pushed the mh/backtrace branch 2 times, most recently from a3b9d61 to 6d71dc4 Compare September 27, 2018 20:00

fingolfin changed the title ~~WIP: overhaul tracking of current statement (DO NOT MERGE OR REVIEW)~~ Overhaul tracking of current statement Sep 27, 2018

fingolfin removed do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) do not review PRs which are not yet ready for a proper external review (e.g. only submitted for test results) labels Sep 27, 2018

fingolfin requested review from markuspf and stevelinton September 27, 2018 20:01

fingolfin commented Sep 27, 2018

View reviewed changes

fingolfin force-pushed the mh/backtrace branch from 6d71dc4 to d2d073f Compare September 27, 2018 20:56

fingolfin commented Sep 27, 2018

View reviewed changes

fingolfin force-pushed the mh/backtrace branch 2 times, most recently from fc17548 to 58101f8 Compare October 4, 2018 20:31

fingolfin force-pushed the mh/backtrace branch from 58101f8 to 7277104 Compare October 5, 2018 10:03

fingolfin force-pushed the mh/backtrace branch from 7277104 to 9704d53 Compare October 19, 2018 09:34

fingolfin added 3 commits October 24, 2018 10:15

Add tst/testspecial/backtrace.g

bfcbcf0

kernel: record current statement in EXEC_STAT

e6f9c1a

This way, we always know which statement was executed last. The price we pay is a small performance overhead.

fingolfin force-pushed the mh/backtrace branch from 9704d53 to 832d7d0 Compare October 24, 2018 09:15

fingolfin added 6 commits October 24, 2018 11:34

gac: stop emiting {RES,OLD,REM,SET}_BRK_CURR_STAT

c06c127

kernel: simplify ExecBegin/ExecEnd

e2fff07

Revert "Make matrix derefs report error locations correctly"

57ca361

This reverts commit 70ee840.

kernel: remove *_BRK_CURR_STAT

711968b

In a few select spaces, we insert `SET_BRK_CALL_TO` calls instead, in order to improve certain backtraces.

kernel: insert a few SET_BRK_CALL_TO(stat) calls

d33bc6c

After evaluating the body of a loop resp. an if-statmeent, and before evaluating the next condition expression, reset the current statement to point to the loop resp. if-statement again.

Verify issue gap-system#2656 is fixed

9764cee

fingolfin force-pushed the mh/backtrace branch from 832d7d0 to 9764cee Compare October 24, 2018 10:35

stevelinton approved these changes Oct 24, 2018

View reviewed changes

ChrisJefferson merged commit da185a7 into gap-system:master Oct 24, 2018

fingolfin deleted the mh/backtrace branch October 24, 2018 15:57

fingolfin changed the title ~~Overhaul tracking of current statement~~ Overhaul tracking of current statement, fixing several bugs where the break loop error message referenced the wrong statement Aug 21, 2019

fingolfin added kind: bug Issues describing general bugs, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 21, 2019

DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019

olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul tracking of current statement, fixing several bugs where the break loop error message referenced the wrong statement #2520

Overhaul tracking of current statement, fixing several bugs where the break loop error message referenced the wrong statement #2520

fingolfin commented Jun 6, 2018 •

edited

Loading

fingolfin Jun 6, 2018

ChrisJefferson Jun 7, 2018

fingolfin Jun 6, 2018

codecov bot commented Jun 6, 2018 •

edited

Loading

fingolfin commented Sep 27, 2018

fingolfin Sep 27, 2018

ChrisJefferson Sep 27, 2018

fingolfin Sep 27, 2018

fingolfin Sep 27, 2018

fingolfin Sep 27, 2018

fingolfin commented Oct 19, 2018

stevelinton left a comment

Overhaul tracking of current statement, fixing several bugs where the break loop error message referenced the wrong statement #2520

Overhaul tracking of current statement, fixing several bugs where the break loop error message referenced the wrong statement #2520

Conversation

fingolfin commented Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 6, 2018 • edited Loading

Codecov Report

fingolfin commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Oct 19, 2018

stevelinton left a comment

Choose a reason for hiding this comment

fingolfin commented Jun 6, 2018 •

edited

Loading

codecov bot commented Jun 6, 2018 •

edited

Loading