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

runtime: include .data section in globals scan #2909

Closed
wants to merge 1 commit into from
Closed

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Jun 15, 2022

All the globals are between _etext and _end. We were scanning only between _edata and _end, which mainly consists of the .bss section. Scanning from _etext ensures that the .data section is also included.

This bug didn't result in issues in CI, but did result in a bug in the recover branch: #2331. This patch fixes this bug.

@deadprogram
Copy link
Member

Previously we used to scan between _edata and _end. This is not correct:
the .data section starts *before* _edata.

Fixing this would mean changing _edata to _etext, but that didn't quite
work either. It appears that there are inaccessible pages between _etext
and _end on ARM. Therefore, a different solution was needed.

What I've implemented is similar to Windows and MacOS: namely, finding
writable segments by parsing the program header of the currently running
program. It's a lot more verbose, but it should be correct on all
architectures. It probably also reduces the globals to scan to those
that _really_ need to be scanned.

This bug didn't result in issues in CI, but did result in a bug in the
recover branch: #2331. This
patch fixes this bug.
@aykevl aykevl force-pushed the gc-globals-data branch from 8e1550c to fc28b11 Compare June 15, 2022 23:11
@aykevl
Copy link
Member Author

aykevl commented Jun 15, 2022

I hope I've really fixed it now. The new patch makes scanGlobals more like Windows and MacOS.

@aykevl
Copy link
Member Author

aykevl commented Jun 15, 2022

Looks like this patch gets #2331 to work! Finally!

Instead of merging this PR, it may be easier to merge #2331, which also contains the commit in this PR (otherwise #2331 would need to be rebased after this PR got merged).

@deadprogram
Copy link
Member

Closing this PR as advised by @aykevl since this commit was merged into dev with PR #2331

@deadprogram deadprogram deleted the gc-globals-data branch May 23, 2023 08:48
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.

2 participants