Skip to content

Fix makeGray misuse in GC (#2178) #2285

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

Closed
wants to merge 2 commits into from

Conversation

HerrCai0907
Copy link
Member

@HerrCai0907 HerrCai0907 commented May 9, 2022

⯈Fixed Template literals cause GC crash #2178
⯈This bug is caused by the misuse of makeGray. makeGray can only be used in STATE_MARK because it will add the object into toSpace, in the sweep state, toSpace is used to store the object which need to free.
⯈Add a needScan function to handle this situation, in the mark state it will call makeGray and in the other state will link the object into fromSpace (just like __unpin did)

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@HerrCai0907 HerrCai0907 changed the title Issue 2178 Fix makeGray misuse in GC (#2178) May 9, 2022
@HerrCai0907
Copy link
Member Author

@dcodeIO bootstrap failed in CI but it works in my env. Do you have any idea?

TypeError: fetch failed
    at Object.processResponse (node:internal/deps/undici/undici:5575:34)
    at node:internal/deps/undici/undici:5901:42
    at node:internal/process/task_queues:140:7
    at AsyncResource.runInAsyncScope (node:async_hooks:202:9)
    at AsyncResource.runMicrotask (node:internal/process/task_queues:137:8) {
  cause: Error: not implemented... yet...
      at makeNetworkError (node:internal/deps/undici/undici:4675:51)
      at schemeFetch (node:internal/deps/undici/undici:5870:18)
      at node:internal/deps/undici/undici:5729:26
      at mainFetch (node:internal/deps/undici/undici:5750:11)
      at fetching (node:internal/deps/undici/undici:5703:7)
      at Agent2.fetch2 (node:internal/deps/undici/undici:5586:20)
      at Object.fetch (node:internal/deps/undici/undici:6372:20)
      at fetch (node:internal/bootstrap/pre_execution:199:25)
      at file:///home/runner/work/assemblyscript/assemblyscript/build/assemblyscript.debug.js:2[23](https://github.com/AssemblyScript/assemblyscript/runs/6345800013?check_suite_focus=true#step:6:23)3:60
      at file:///home/runner/work/assemblyscript/assemblyscript/build/assemblyscript.debug.js:22[38](https://github.com/AssemblyScript/assemblyscript/runs/6345800013?check_suite_focus=true#step:6:38):3 {
    [cause]: undefined
  }
}

@MaxGraey
Copy link
Member

MaxGraey commented May 9, 2022

bootstrap failed in CI but it works in my env. Do you have any idea?

It's something related to fetch in latest node

@dcodeIO
Copy link
Member

dcodeIO commented May 15, 2022

It seems to me that what __unpin does is relatively specific to __unpin, in that it typically moves an object from pinSpace back to a non-pinSpace, and makeGray is conveniently used to avoid issues in part of the mark phase. The inverse change to the write barrier, however, looks as if it changes how incremental marking works during the entire mark phase, and I am having a hard time wrapping my head around the effects. From the comment in __unpin I'd assume that if the cause has been identified correctly, a similar fix in the mark phase should only apply in the specific case of "right at the point of marking roots for the second time" but otherwise leaving the write barrier intact, which makes me wonder whether there should be a second-stage mark phase that the change can be limited to or whether an atomic finalize step is necessary as is often used in similar implementations. Perhaps, can you help me to understand the full effects of the change to the mark phase?

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented May 16, 2022

The color for toSpace and fromSpace in difference state:
STATE_IDLE:
fromSpace: white (created by __new)
toSpace: gray | black(pointer_free)

STATE_MARK:
fromSpace: white (created by __new)
toSpace: gray | black, will change to black at end.

STATE_SWEEP: before this state, switch fromSpace and toSpace, black and white
fromSpace: new white
toSpace: new black

In STATE_SWEEP, __unpin and __link should not use makeGray.

In the most of case, it is impossible that the child is white but parent is black, so the __link will do nothing in STATE_SWEEP. However template literals will use Static Array to join substring, and the static array isn't store in any reachable structure except shadow stack. It means after stack going back, the gcinfo stored in Static Array will not be updated. If white/black switch happened during the 2 times usage of same template string, unfortunately misuse of makeGray will happened because the color stored in Static Array is old.

Updating GC algorithm to make it more robust is a solution. Another solution is change the gcinfo of Static Array generated by compiler. see PR #2295

@HerrCai0907
Copy link
Member Author

Fixed by #2295

@HerrCai0907 HerrCai0907 deleted the issue-2178 branch June 24, 2022 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants