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

kernel: access TmpPPerm via ADDR_PPERM4 #2495

Merged
merged 1 commit into from
May 28, 2018

Conversation

fingolfin
Copy link
Member

Fixes #2493

Not for release notes, as this does not actually effect GASMAN or Boehm GC; only with Julia GC will it play a role.

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels May 28, 2018
@codecov
Copy link

codecov bot commented May 28, 2018

Codecov Report

Merging #2495 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2495      +/-   ##
==========================================
+ Coverage   74.27%   74.27%   +<.01%     
==========================================
  Files         484      484              
  Lines      245340   245343       +3     
==========================================
+ Hits       182215   182218       +3     
  Misses      63125    63125
Impacted Files Coverage Δ
src/pperm.c 97.89% <100%> (ø) ⬆️
src/stats.c 89.41% <0%> (-0.2%) ⬇️
src/hpc/threadapi.c 43.48% <0%> (-0.11%) ⬇️
src/pperm.h 100% <0%> (ø) ⬆️
src/gasman.c 85.86% <0%> (+0.17%) ⬆️
hpcgap/lib/hpc/stdtasks.g 64.96% <0%> (+0.25%) ⬆️

Copy link
Contributor

@james-d-mitchell james-d-mitchell left a comment

Choose a reason for hiding this comment

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

This looks ok to me, mod the following comment. Given that the first two sizeof(Obj) in TmpPPerm were previously being used, are you sure that enough space is being allocated by ResizeTmpPerm before it is used? I.e. this change effectively makes TmpPPerm shorter by 2 * sizeof(Obj) so it would be good to verify that there is no out of bounds accesses caused by this change.

@fingolfin
Copy link
Member Author

@james-d-mitchell yes, I checked for that, and the resizing code for the tmpperm already added these two Obj slots, plus an extra UInt4 slot (for the degree).

@fingolfin fingolfin merged commit fa92ce7 into gap-system:master May 28, 2018
@fingolfin fingolfin deleted the mh/fix-TmpPPerm branch May 28, 2018 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants