wasm2c: Fix handling of locals in setjmp targets #2479
Conversation
|
(the tests would catch this, despite UBSAN not having a setjmp sanitizer, because CI runs tests on both debug and release builds) (upstreaming is proving difficult... WebAssembly/exception-handling#332 ) |
I'm not quite sure what you are saying here. Do you mean the new test you are adding would fail without the fix?
|
| if (setjmp_safe) { | ||
| PushFuncSection("exceptions"); | ||
| Write("volatile "); | ||
| PushFuncSection(); |
There was a problem hiding this comment.
I assume this means we only output this volatile when exceptions are enabled?
There was a problem hiding this comment.
also, only when a try block is used in the function. (i.e. only when setjmp is called.)
the test would fail without the fix, yes. like so: (note the |
| for (size_t i = 0; i < func_sections_.size(); ++i) { | ||
| auto& [condition, stream] = func_sections_.at(i); | ||
| std::unique_ptr<OutputBuffer> buf = stream.ReleaseOutputBuffer(); | ||
| if (condition.empty() || func_includes_.count(condition)) { | ||
| stream_->WriteData(buf->data.data(), buf->data.size()); | ||
| } | ||
|
|
||
| if (i == 0) { | ||
| if (i == stack_var_section) { |
There was a problem hiding this comment.
Is the stack_var_section not always the first one in a given function?
There was a problem hiding this comment.
not with these changes. we need to write volatile or not based on whether the function uses try or not, and we do that by pushing more func sections where relevant. in particular, we save the stack_var_section after writing params and locals (both of which may contain volatile), so we can put the stack vars in the correct place. (also, stack vars don't need volatile as any reused stack slots are overwritten after longjmp.)
It is UB to read local variables after a call to
setjmpreturns, if those variables have been modified betweensetjmpandlongjmp, unless they're marked asvolatile. This marks them asvolatile.Closes #2469
Split from #2470