GeezerVison

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Pyodide gives much better stack traces on this sort of failure. I think one thing it does is call _Py_DumpTraceback(). I'll try to add that logic.

When I ran the tests locally with this fix I think they all passed, though I didn't pass -uall.

When I ran the tests locally with this fix I think they all passed, though I didn't pass -uall.

I've been able to reproduce this failure locally (on macOS) and on the buildbot machine (linux).

Indeed, I can reproduce it too. Maybe let's just add @support.skip_emscripten_stack_overflow() to test_infinite_cycle_in_bases for now, it's only one more recursion test that is broken...

With the fix from #131158 applied, running on macOS, the test suite completes, but I get three test failures (test_os, test_shutil and test_unicode_file_functions). However, they all appear to be for filesystem access reasons, unrelated to this PR.

I'm also seeing failures with -uall related in test_builtin, test_capi, test_descr; there are probably others; that's as far as I got with manually disabling single tests with -x. However, as with the test_isinstance failure, I get the impression that these are more traditional stack overflow issues that need @support.skip_emscripten_stack_overflow() markers (some of them already have skip_wasi_stack_overflow). We can clean up those and the filesystem tests in a separate PR.

The new stack traces are really pretty though 😄

So - I'm happy to sign off on this; once we've got confirmation from @markshannon that he doesn't object to the approach, we can merge and we're one step closer to a buildbot!

The fix for this just barely missed inclusion into Emscripten 4.0.5 but it will definitely be in 4.0.6.

This approach seems fine to me.

Protecting against stack overflow is very much a "practicality beats purity" thing.
We want extremely few, ideally no, cases where the VM crashes, and within that constraint we want to maximize the available stack available.

I used pthread_get_stackaddr_np because it happens to work for linux and similar systems. If there is something else that works better for the webassembly platforms, please feel free to use that.

The goal of Emscripten is to behave as much like linux as possible, subject to other the architecture and platform constraints and the desire to minimize code size. So in cases like this, finding discrepancies with normal linux and resolving them in Emscripten is ideal.

If there were a cheap way to ask about the true stack in addition to the spill stack, that would be great but as it is at least we can ask about the spill stack correctly. Maybe now if we set the size of the spill stack to be kind of small the stack checks will work a bit more often.