Skip to content

Fix overflow with optimized WASM code by ensuring that globals are in… #25

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

Merged
jplevyak merged 1 commit into from
Feb 21, 2019
Merged

Fix overflow with optimized WASM code by ensuring that globals are in… #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 1 addition & 3 deletions source/extensions/common/wasm/wasm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,12 +806,10 @@ bool Context::isSsl() { return decoder_callbacks_->connection()->ssl() != nullpt
// Calls into the WASM code.
//
void Context::onStart() {
wasm_->wasmVm()->start(this);
Copy link

@lambdai lambdai Feb 20, 2019

Choose a reason for hiding this comment

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

Any test to cover the behavior?

Copy link

@lambdai lambdai Feb 20, 2019

Choose a reason for hiding this comment

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

ensuring that globals are initialized. I get it. Thanks!

Copy link
Author

@jplevyak jplevyak Feb 20, 2019

Choose a reason for hiding this comment

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

This adds -O3 to a WASM file for a test that used to cause a crash with -O3, so that is the test.

if (wasm_->onStart_) {
wasm_->onStart_(this);
return;
}
// Fall back to any start or main() function.
wasm_->wasmVm()->start(this);
}

void Context::onConfigure(absl::string_view configuration) {
Expand Down
10 changes: 9 additions & 1 deletion source/extensions/common/wasm/wavm/wavm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,16 @@ bool Wavm::initialize(const std::string& code, absl::string_view name, bool allo

void Wavm::start(Context* context) {
Copy link

@PiotrSikora PiotrSikora Feb 23, 2019

Choose a reason for hiding this comment

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

@jplevyak unless I'm mistaken, this is never called in program, only in tests, which means that we neither initialize globals, nor call the start function (or main()), and starting program with optimized WASM module leads to a crash.

I think we've lost the call to start() during one of the refactors. Could you look into this? Thanks!

auto f = getStartFunction(moduleInstance_);
if (!f)
if (f) {
CALL_WITH_CONTEXT(invokeFunctionChecked(context_, f, {}), context);
Copy link

@lambdai lambdai Feb 20, 2019

Choose a reason for hiding this comment

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

Is there a early return? It's possible that f is reassigned and invoked at line 287

Copy link
Author

@jplevyak jplevyak Feb 20, 2019

Choose a reason for hiding this comment

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

So, we want to call the generic start function before the global variables are initialized. I am not sure why, but that is what WAVM does and this is WAVM specific code. The "start" function is special and according to the docs should be called (if available) before any exported functions.

Copy link

@lambdai lambdai Feb 21, 2019

Choose a reason for hiding this comment

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

Thank you for the explanation!

}
if (emscriptenInstance_) {
Emscripten::initializeGlobals(context_, irModule_, moduleInstance_);
}
f = asFunctionNullable(getInstanceExport(moduleInstance_, "main"));
Copy link

@PiotrSikora PiotrSikora Feb 20, 2019

Choose a reason for hiding this comment

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

Actually, on a second thought, what if main() is exported as the start function? Could we guard against that and not call it twice?

Copy link
Author

@jplevyak jplevyak Feb 20, 2019

Choose a reason for hiding this comment

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

AFAICT the "start" function is special and according to the docs should be called (if available) before any exported functions e.g. "main" or "_main". In this case it is also called before the Emscripten module globals are initialized because that is what wavm-run does and this is WAVM specific code.

Copy link

@PiotrSikora PiotrSikora Feb 21, 2019

Choose a reason for hiding this comment

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

Yeah, my point was that main() function might be marked as the "start" function (at the WASM level), in which case we'd call it twice... but maybe it wouldn't be exported then, though.

Anyway, we can deal with it if that becomes an issue with other languages.

if (!f) {
f = asFunctionNullable(getInstanceExport(moduleInstance_, "_main"));
}
if (f) {
CALL_WITH_CONTEXT(invokeFunctionChecked(context_, f, {}), context);
}
Expand Down
6 changes: 1 addition & 5 deletions test/extensions/filters/http/wasm/test_data/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@ API=../../../../../../api/wasm/cpp

EMSCRIPTEN_OPT=-s WASM=1 -s EMIT_EMSCRIPTEN_METADATA=1

# Note that optimizations are disabled, because optimized WASM module
# throws wavm.integerDivideByZeroOrOverflow, possibly due to an issue
# with getFunctionWavm() templates and/or their usage.
#CXXFLAGS=--std=c++14 -O3 -g3
CXXFLAGS=--std=c++14 -g3
CXXFLAGS=--std=c++14 -O3 -g3
Copy link

@lambdai lambdai Feb 20, 2019

Choose a reason for hiding this comment

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

-g3: TIL. Thanks!


all: headers.wasm async_call.wasm

Expand Down
Binary file modified test/extensions/filters/http/wasm/test_data/async_call.wasm
View file
Open in desktop
Binary file not shown.
68,387 changes: 16,784 additions & 51,603 deletions test/extensions/filters/http/wasm/test_data/async_call.wat
View file
Open in desktop

Large diffs are not rendered by default.

Binary file modified test/extensions/filters/http/wasm/test_data/headers.wasm
View file
Open in desktop
Binary file not shown.
58,311 changes: 15,014 additions & 43,297 deletions test/extensions/filters/http/wasm/test_data/headers.wat
View file
Open in desktop

Large diffs are not rendered by default.