Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-128627: Emscripten: Fix address calculation for wasm-gc trampoline#128782
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
…polineWe need to divide the entire memory address by 4, not just the base address.
| constoffset=HEAP32[__PyEM_EMSCRIPTEN_COUNT_ARGS_OFFSET/4]; | ||
| HEAP32[__PyRuntime/4+offset]=ptr; | ||
| constoffset=HEAP32[__PyEM_EMSCRIPTEN_COUNT_ARGS_OFFSET /4]; | ||
| HEAP32[(__PyRuntime+offset) /4]=ptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This line is the only change:base/4 + offset ==>(base + offset)/4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the memory calculation makes sense; I guess the biggest question is how far off being able to spool up a buildbot are we? I'd feel a lot more confident about changes like this if we were in a position to do comprehensive testing on each change.
One other question about a change that isn't in the PR description; it seems likely that it's intentional, but I want to make sure.
| AS_VAR_APPEND([LINKFORSHARED],[" -sFORCE_FILESYSTEM -lidbfs.js -lnodefs.js -lproxyfs.js -lworkerfs.js"]) | ||
| AS_VAR_APPEND([LINKFORSHARED],[" -sEXPORTED_RUNTIME_METHODS=FS,callMain,ENV"]) | ||
| AS_VAR_APPEND([LINKFORSHARED],[" -sEXPORTED_FUNCTIONS=_main,_Py_Version"]) | ||
| AS_VAR_APPEND([LINKFORSHARED],[" -sEXPORTED_FUNCTIONS=_main,_Py_Version,__PyRuntime,__PyEM_EMSCRIPTEN_COUNT_ARGS_OFFSET"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This doesn't appear to be related to code formatting or the correction to memory address calculation - is it intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah,__PyRuntime and__PyEM_EMSCRIPTEN_COUNT_ARGS_OFFSET are accessed from JS and that won't work without this.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Well, there is one file with a bunch of failures that will be fixed byemscripten-core/emscripten#23364 and there are a few failures that require stating a pipe. I was planning to xfail both of them for now because the changes are taking a long time in review. But some time between January 6th and January 13th my local builds started segfaulting on startup. Even if I check out an old "known-good" Python commit and use an old "known-good" Emscripten version, it always segfaults. It's a bit of a mystery what changed, I'll have to make a debug build and chase down what's going on. Anyways, once I fix that I think I can make one more PR changing some test skips and then can make a build bot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Approving on the basis that the outstanding question has been resolved. If there's followups needed to address the segfault you describe, we can handle that as a separate PR.
960936f intopython:mainUh oh!
There was an error while loading.Please reload this page.
To be clear I don't think there is a regression that is checked into the repository. I suspect that something is going wrong specific to my machine, like a poisoned cache somewhere. |
Indeed, starting from a fresh checkout of everything I can build again segfault free. =) |
Uh oh!
There was an error while loading.Please reload this page.
We need to divide the entire memory address by 4, not just the base address. I also did a little code formatting while I'm at it.