8000 fix #1042 by aryairani · Pull Request #1183 · unisonweb/unison · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix #1042 #1183

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jan 29, 2020
Merged

fix #1042 #1183

merged 11 commits into from
Jan 29, 2020

Conversation

aryairani
Copy link
Contributor
@aryairani aryairani commented Jan 29, 2020 •
edited
Loading

Problem: #1042 reported invalid arguments being passed to MV.slice:

    -- Garbage collect the elements of the stack that are more than `maxSlot`
    -- below the top - this is done just by copying to a fresh stack.
    gc :: Size -> Stack -> Int -> IO (Size, Stack)
    gc size m maxSlot = do
      -- ...
      let m2 = MV.slice (size - maxSlot - 1) size2 m
      -- ...

^ There is the offending slice. Variable values:

maxSlot = 129
size = 129

This is surprising because afaik maxSlot 0-indexes into a vector of size size, thus maxSlot < size.

Solution:

We were calling

Let var b body freeInBody ->
  -- ...
  gc size m (if Set.null freeInBody then 0 else Set.findMax freeInBody)
  -- ...

and both cases of the if are wrong.

If Set.null freeInBody then we should pass -1 8000 , indicating that no elements from the current stack are needed, and canceling the - 1 in the slice start index calculation.

For the "else" case, the De Bruijn indices stored in freeInBody are relative to the not-yet-constructed stack, which includes evaluated let binding. Since it's not yet part of the stack we are garbage collecting from, we need to subtract one from the max. So in the example above, maxSlot=129 was wrong; it should have been 128, so we'd get a slice start index of 0 (129 - 128 - 1) instead of -1.

We end up with:

let maxSlot =
  if Set.null freeInBody then -1
  else Set.findMax freeInBody - 1
in gc size m maxSlot

@aryairani aryairani marked this pull request as ready for review January 29, 2020 22:11
@aryairani aryairani requested a review from pchiusano January 29, 2020 22:11
pchiusano
pchiusano approved these changes Jan 29, 2020
View reviewed changes
Copy link
Member
@pchiusano pchiusano left a comment

Choose a reason for hiding this comment

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

LGTM

@pchiusano pchiusano added the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Jan 29, 2020
@mergify mergify bot merged commit 44ed1db into master Jan 29, 2020
@aryairani aryairani removed the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Jan 30, 2020
@aryairani aryairani deleted the fix/1042-big-list-crashes branch January 30, 2020 00:23
@aryairani aryairani mentioned this pull request Mar 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchiusano pchiusano pchiusano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aryairani @pchiusano @galer1us
0