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

Fix crash when comparing recursive data structures #1151

Merged
merged 3 commits into from
Feb 21, 2017

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Feb 16, 2017

This fixes crashes when comparing recursive data structures (see #1150). With this PR, we get these errors (instead of crashes):

gap> [~] < [~];
Error, recursion depth trap (5000)

not in any function at *stdin*:1
you may 'return;'
brk>

and

gap> rec(a:=~) = rec(a:=~);
Error, recursion depth trap (5000)

not in any function at *stdin*:1
you may 'return;'
brk>

etc. etc.

Note that the user can still choose to continue the recursion, and doing so often enough will still crash. That seems acceptable to me, but if desired, we could also produce a variant of this code which prevents the user from returning here.

Also, we should add test cases for these (former) crashes.

Finally, I wonder if other function share similar problems? E.g. are there lists/record types which are not plists / precords and which can also be recursive?

@codecov
Copy link

codecov bot commented Feb 16, 2017

Codecov Report

Merging #1151 into master will decrease coverage by -0.03%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##           master    #1151      +/-   ##
==========================================
- Coverage   59.27%   59.24%   -0.03%     
==========================================
  Files         434      435       +1     
  Lines      231096   231106      +10     
==========================================
- Hits       136972   136920      -52     
- Misses      94124    94186      +62
Impacted Files Coverage Δ
src/funcs.h 100% <100%> (ø)
src/plist.c 91.69% <100%> (+0.02%)
src/funcs.c 86.72% <100%> (+0.98%)
src/precord.c 89.2% <93.33%> (+0.56%)
src/permutat.c 55.21% <ø> (-2.92%)
lib/tietze.gi 41.85% <ø> (-0.74%)
lib/grpmat.gi 70.89% <ø> (-0.56%)
lib/straight.gi 25.28% <ø> (-0.3%)
src/compiler.c 65.45% <ø> (-0.05%)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f90e2ad...5ededb1. Read the comment docs.

Copy link
Contributor

@ChrisJefferson ChrisJefferson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add some tests. There are in my mind two types to add:

  1. Some tests with some ~ in
  2. Some tests where you just do a bunch of comparision of some nested lists and records in a loop of 10,000 iterations or so, where the code has to do some recursion, to make sure we are incrementing/decrementing the recursiondepth correctly.

However, the general design is fine.

src/plist.c Outdated
return 0L;
}
else if ( ! EQ( elmL, elmR ) ) {
if ( (elmL == 0 ) != (elmR == 0) || ! EQ( elmL, elmR ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I be picky and ask for another pair of brackets here? I should know what the priority order of != and || is, but I'm afraid I can't remember. However, I agree the rearranged code is fine.

}

}

/* the records are equal or the right is a prefix of the left */
return False;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pop TLS RecusionDepth?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely. This is one place where I'd love we were in C++ and could manage this via RAII

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Note that you commented the old version of the code, not the new, hence the fix does not show up here)

@fingolfin fingolfin force-pushed the mh/recursion branch 2 times, most recently from 938ca61 to 5ededb1 Compare February 18, 2017 08:10
@fingolfin
Copy link
Member Author

@ChrisJefferson I am not quite sure I understand what you want to do with your second test... But I suggest you add it yourself once this PR has been merged?

@ChrisJefferson
Copy link
Contributor

@fingolfin Will do. Apart from that, I'm now happy with the PR.

@olexandr-konovalov olexandr-konovalov merged commit e37d037 into gap-system:master Feb 21, 2017
@fingolfin fingolfin deleted the mh/recursion branch February 27, 2017 14:28
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants