gh-128627: Emscripten: Fix address calculation for wasm-gc trampoline #128782
Conversation
…poline We need to divide the entire memory address by 4, not just the base address.
| const offset = HEAP32[__PyEM_EMSCRIPTEN_COUNT_ARGS_OFFSET/4]; | ||
| HEAP32[__PyRuntime/4 + offset] = ptr; | ||
| const offset = HEAP32[__PyEM_EMSCRIPTEN_COUNT_ARGS_OFFSET / 4]; | ||
| HEAP32[(__PyRuntime + offset) / 4] = ptr; |
There was a problem hiding this comment.
This line is the only change: base/4 + offset ==> (base + offset)/4.
There was a problem hiding this comment.
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.
| @@ -2369,7 +2369,7 @@ AS_CASE([$ac_sys_system], | |||
| dnl Include file system support | |||
| 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.
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.
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 by emscripten-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. |
|
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. =) |
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.