aheejin wrote: > > > In terms of getting this landed and tested, I wonder which path we should > > > take: > > > > > > 1. Land this now, without tests, then update emscripten then come back > > > and flip the default, at which point the existing tests will get updated. > > > 2. Duplicate/update the the existing tests to tests both modes, then > > > delete those changes once we flip the default. > > > > > > Personally I think I'd be happy with (1) since this is a behind and > > > experimental flag. > > > What do others think? @aheejin ? > > > > > > Come to think of it, should we even introduce this experimental option? > > Adding `if (NewOption) ... else ...` everywhere makes the code complicated. > > Can we just do > > > > 1. Add library functions in emscripten > > So you think it should be possible to have both old and new versions > supported in emscripten at the same time? If that works that would be great. > Do you want to send and emscripten PR? And link to it here? If we can confirm > that all tests pass then I agree it would be great to do this in a single > LLVM change.
No, 1 just adds new library functions to emscripten (with different names than the current ones) and at this point they will not be actually used. The reason we add them there first is otherwise 2 (replacing the current scheme with the new one) wouldn't pass in the emscripten CI. As long as we use different library function names I think it can be fine. (I actually want to reuse the name `__wasm_longjmp` (instead of `__wasm_sjlj_longjmp` this PR uses), but maybe we can change that after landing all these.) @yamt, do you want to submit a PR to emscripten adding new library functions? I can do that too, but given that this is your code, if you want to do it it'd be good. About where to add them... to make the transition smooth, https://github.com/emscripten-core/emscripten/blob/main/system/lib/compiler-rt/emscripten_setjmp.c looks like a good place to start given that our current functions are here, but we can move them later to elsewhere. WDYT? @sbc100 https://github.com/llvm/llvm-project/pull/84137 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
