Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wasm2c: Fix handling of locals in setjmp targets #2479

Merged
merged 2 commits into from
Oct 2, 2024
Merged

wasm2c: Fix handling of locals in setjmp targets #2479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26ad21c
wasm2c: Fix handling of locals in setjmp targets
SoniEx2 Sep 26, 2024
0a02215
Add test
SoniEx2 Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 35 additions & 14 deletions src/c-writer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,11 +426,12 @@ class CWriter {
void WriteImportProperties(CWriterPhase);
void WriteFuncs();
void BeginFunction(const Func&);
void FinishFunction();
void FinishFunction(size_t);
void Write(const Func&);
void WriteTailCallee(const Func&);
void WriteParamsAndLocals();
void WriteParams(const std::vector<std::string>& index_to_name);
void WriteParams(const std::vector<std::string>& index_to_name,
bool setjmp_safe = false);
void WriteParamSymbols(const std::vector<std::string>& index_to_name);
void WriteParamTypes(const FuncDeclaration& decl);
void WriteLocals(const std::vector<std::string>& index_to_name);
Expand All @@ -444,7 +445,7 @@ class CWriter {
void Unspill(const TypeVector&);

template <typename Vars, typename TypeOf, typename ToDo>
void WriteVarsByType(const Vars&, const TypeOf&, const ToDo&);
void WriteVarsByType(const Vars&, const TypeOf&, const ToDo&, bool);

template <typename sources>
void Spill(const TypeVector&, const sources& src);
Expand Down Expand Up @@ -3043,15 +3044,15 @@ void CWriter::BeginFunction(const Func& func) {
Write(Newline());
}

void CWriter::FinishFunction() {
void CWriter::FinishFunction(size_t stack_var_section) {
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the stack_var_section not always the first one in a given function?

Copy link
Contributor Author

@SoniEx2 SoniEx2 Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

WriteStackVarDeclarations(); // these come immediately after section #0
// (return type/name/params/locals)
}
Expand All @@ -3071,6 +3072,7 @@ void CWriter::Write(const Func& func) {
WriteParamsAndLocals();
Write("FUNC_PROLOGUE;", Newline());

size_t stack_vars_section = func_sections_.size() - 1;
PushFuncSection();

std::string label = DefineLabelName(kImplicitFuncLabel);
Expand All @@ -3094,13 +3096,14 @@ void CWriter::Write(const Func& func) {
}

stream_ = prev_stream;
FinishFunction();
FinishFunction(stack_vars_section);
}

template <typename Vars, typename TypeOf, typename ToDo>
void CWriter::WriteVarsByType(const Vars& vars,
const TypeOf& typeoffunc,
const ToDo& todo) {
const ToDo& todo,
bool setjmp_safe) {
for (Type type : {Type::I32, Type::I64, Type::F32, Type::F64, Type::V128,
Type::FuncRef, Type::ExternRef}) {
Index var_index = 0;
Expand All @@ -3109,6 +3112,11 @@ void CWriter::WriteVarsByType(const Vars& vars,
if (typeoffunc(var) == type) {
if (count == 0) {
Write(type, " ");
if (setjmp_safe) {
PushFuncSection("exceptions");
Write("volatile ");
PushFuncSection();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this means we only output this volatile when exceptions are enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep!

Copy link
Contributor Author

@SoniEx2 SoniEx2 Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, only when a try block is used in the function. (i.e. only when setjmp is called.)

}
Indent(4);
} else {
Write(", ");
Expand Down Expand Up @@ -3144,14 +3152,15 @@ void CWriter::WriteTailCallee(const Func& func) {
if (func.GetNumParams()) {
WriteVarsByType(
func.decl.sig.param_types, [](auto x) { return x; },
[&](Index i, Type) { Write(DefineParamName(index_to_name[i])); });
[&](Index i, Type) { Write(DefineParamName(index_to_name[i])); }, true);
Write(OpenBrace(), func.decl.sig.param_types, " tmp;", Newline(),
"wasm_rt_memcpy(&tmp, tail_call_stack, sizeof(tmp));", Newline());
Unspill(func.decl.sig.param_types,
[&](auto i) { return ParamName(index_to_name[i]); });
Write(CloseBrace(), Newline());
}

size_t stack_vars_section = func_sections_.size() - 1;
WriteLocals(index_to_name);

PushFuncSection();
Expand All @@ -3175,19 +3184,20 @@ void CWriter::WriteTailCallee(const Func& func) {
Write("next->fn = NULL;", Newline());

stream_ = prev_stream;
FinishFunction();
FinishFunction(stack_vars_section);
}

void CWriter::WriteParamsAndLocals() {
std::vector<std::string> index_to_name;
MakeTypeBindingReverseMapping(func_->GetNumParamsAndLocals(), func_->bindings,
&index_to_name);
WriteParams(index_to_name);
WriteParams(index_to_name, true);
Write(" ", OpenBrace());
WriteLocals(index_to_name);
}

void CWriter::WriteParams(const std::vector<std::string>& index_to_name) {
void CWriter::WriteParams(const std::vector<std::string>& index_to_name,
bool setjmp_safe) {
Write(ModuleInstanceTypeName(), "* instance");
if (func_->GetNumParams() != 0) {
Indent(4);
Expand All @@ -3196,7 +3206,13 @@ void CWriter::WriteParams(const std::vector<std::string>& index_to_name) {
if (i != 0 && (i % 8) == 0) {
Write(Newline());
}
Write(func_->GetParamType(i), " ", DefineParamName(index_to_name[i]));
Write(func_->GetParamType(i), " ");
if (setjmp_safe) {
PushFuncSection("exceptions");
Write("volatile ");
PushFuncSection();
}
Write(DefineParamName(index_to_name[i]));
}
Dedent(4);
}
Expand Down Expand Up @@ -3240,13 +3256,17 @@ void CWriter::WriteLocals(const std::vector<std::string>& index_to_name) {
} else {
Write("0");
}
});
},
true);
}

void CWriter::WriteStackVarDeclarations() {
// NOTE: setjmp_safe must be false, can't push func sections in here because
// this is called from FinishFunction.
// (luckily, we don't need setjmp_safe for these.)
WriteVarsByType(
stack_var_sym_map_, [](auto stp_name) { return stp_name.first.second; },
[&](Index, auto& stp_name) { Write(stp_name.second); });
[&](Index, auto& stp_name) { Write(stp_name.second); }, false);
}

void CWriter::Write(const Block& block) {
Expand All @@ -3262,6 +3282,7 @@ void CWriter::Write(const Block& block) {
}

size_t CWriter::BeginTry(const TryExpr& tryexpr) {
func_includes_.insert("exceptions");
Write(OpenBrace()); /* beginning of try-catch */
const std::string tlabel = DefineLabelName(tryexpr.block.label);
Write("WASM_RT_UNWIND_TARGET *", tlabel,
Expand Down
25 changes: 25 additions & 0 deletions test/regress/wasm2c-ehv3-setjmp-volatile.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
;;; TOOL: run-spec-wasm2c
;;; ARGS*: --enable-exceptions
;;; NOTE: ref: issue-2469
(module
(tag $e0)
(func $longjmp-bait (throw $e0))
(func (export "setjmp-bait") (param $return-early i32) (result i32)
(local $value i32)
(try $try
(do
(br_if $try (local.get $return-early))
(local.set $value (i32.const 1))
(call $longjmp-bait)
)
(catch $e0)
)
(local.get $value)
)
)

(assert_return (invoke "setjmp-bait" (i32.const 1)) (i32.const 0))
(assert_return (invoke "setjmp-bait" (i32.const 0)) (i32.const 1))
(;; STDOUT ;;;
2/2 tests passed.
;;; STDOUT ;;)
Loading