feat(wasm) add support for detailed backtraces #259
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
main -> feat/gateway merge just done; rebasing this branch will significantly improve CI runs |
hishamhm
force-pushed
the
feat/backtraces
branch
10 times, most recently
from
b0aae44 to
54a0c1c
Compare
thibaultcha
reviewed
Apr 10, 2023
hishamhm
force-pushed
the
feat/backtraces
branch
from
54a0c1c to
d322dd6
Compare
|
@thibaultcha thanks for the first round of reviews! I left a bunch of replies, merged the trivial changes and will continue applying the other changes I marked with ":+1:" tomorrow |
hishamhm
force-pushed
the
feat/backtraces
branch
8 times, most recently
from
a316b59 to
7e593fe
Compare
|
Update:
I will resume writing new tests for the |
hishamhm
added a commit
that referenced
this pull request
Apr 13, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
Replace "semi-external dependency" cwabt (a library whose source lives in the ngx_wasm_module repository but which was not built as part of the ngx_wasm_module build process) with a properly "vendored-in" dependency ngx-wasm-rs, which implements the same functionality as a more general Rust library for ngx_wasm_module, which will allow us to add more Rust functionality over time with little deployment overhead.
hishamhm
added a commit
that referenced
this pull request
Apr 13, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
hishamhm
force-pushed
the
feat/backtraces
branch
from
104038b to
bda8f79
Compare
hishamhm
added a commit
that referenced
this pull request
Apr 13, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
hishamhm
force-pushed
the
feat/backtraces
branch
2 times, most recently
from
318a792 to
deb3933
Compare
hishamhm
added a commit
that referenced
this pull request
Apr 13, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
hishamhm
force-pushed
the
feat/backtraces
branch
from
deb3933 to
db8ee4d
Compare
|
Ohhhh ok my bad, I thought —prefix was choosing a default -p (working prefix) directory but yes, no at all it is the install directory ok yes. That does seem much much better, probably what we needed! On Apr 13, 2023, at 10:04 PM, Hisham Muhammad ***@***.***> wrote:
@hishamhm commented on this pull request.
In config:
@@ -20,22 +23,27 @@ case $ngx_wasm_runtime_name in
wasmer)
ngx_wasm_runtime_srcs="$ngx_addon_dir/src/wasm/wrt/ngx_wrt_wasmer.c"
+ ngx_wasm_rust_library=yes
+ ngx_wasm_rust_mandatory=no
+ ngx_wasm_rust_cargo_flags=""
+ ngx_wasm_rust_cargo_crate_type="cdylib"
That creating a prefix directory requires finding and copying the shared lib as well isn't the idiomatic Nginx experience
That depends what you mean by nginx prefix -- if you mean a "working prefix" as in nginx -p /myprefix (containing its own conf, logs, etc.) that would certainly still work: a binary package built with --prefix=/usr/local/nginx will install /usr/local/nginx/sbin/nginx and /usr/local/nginx/lib/libngx_wasm_rs.so (as well as all the other files installed by nginx's make install under /usr/local/nginx), but the nginx binary itself can then be copied anywhere else and it will still work (because the rpath is stored inside the binary itself, it's not relative to the binary's location or the running -p prefix).
That creating a prefix directory requires finding and copying the shared lib as well isn't the idiomatic Nginx experience.
That would not be necessary for a newly-created -p prefix. In fact, it would be the opposite: what is necessary is that the shared lib is not moved around and that it stays in its original make install prefix as specified by the rpath (the binary can be copied around). Given any managed package (.deb, .rpm, etc) that is a very simple assumption to keep.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
This refactor is needed because Wasmtime needs to: 1. read the ngx_wavm_conf_t configuration 2. set up the environment variable 3. create the wasm_config_t object ...in that order. With this small API change, we give the wrt backends the extra flexibility to be able to do that.
hishamhm
force-pushed
the
feat/backtraces
branch
2 times, most recently
from
fcb43bb to
24de344
Compare
Attempt enabling 'llvm', 'cranelift' or 'singlepass', based on compiler availability, in that order.
Previously to this commit, this was the state of backtrace support on panics for our supported VMs: * Wasmtime: outputs backtraces with filenames * Wasmer: no backtraces * V8: no backtraces This adds a new configuration directive, `backtraces on`, which adds support for more detailed backtraces: * Wasmtime: outputs backtraces with function names, filenames and line numbers * Wasmer: outputs backtraces with function names * V8: outputs backtraces with function names The default is `backtraces off`, which is the same as the old behavior (incurring in no additional initialization time for parsing the function name table for Wasmer and V8).
hishamhm
force-pushed
the
feat/backtraces
branch
2 times, most recently
from
47a7937 to
4226eb5
Compare
hishamhm
force-pushed
the
feat/backtraces
branch
from
4226eb5 to
1380ee6
Compare
|
@hishamhm Are you ready for a merge on this one? I see everything is ticked and tests seem to be ready! If there's nothing for you to add I'll go ahead; any minor edits I can do myself. |
|
@thibaultcha yes, it's ready IMO! |
thibaultcha
pushed a commit
that referenced
this pull request
Apr 18, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
thibaultcha
pushed a commit
that referenced
this pull request
Apr 18, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
thibaultcha
pushed a commit
that referenced
this pull request
Apr 19, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
thibaultcha
pushed a commit
that referenced
this pull request
Apr 19, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
|
Merged in |
thibaultcha
pushed a commit
that referenced
this pull request
Apr 19, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
thibaultcha
pushed a commit
that referenced
this pull request
Apr 19, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
thibaultcha
pushed a commit
that referenced
this pull request
Apr 20, 2023
There's a tricky thing going on. Since Wasmer itself is written in Rust and we build using a precompiled libwasmer.a generated with a different Rust version, if we link both libwasmer.a and libngx_wasm_rs.a, we get a link failure with some duplicated incompatible symbols [1][2]. For this reason, when we enable ngx_wasm_rs with Wasmer, we build it as a .so file rather than a statically-linked .a file. With this commit, we can install cdylibs under nginx's `--prefix` at `/lib` (e.g. `/usr/local/nginx/lib`, next to `/usr/local/nginx/sbin/nginx`) and ensure that the `nginx` binary can find it. See here for more info: [1] https://users.rust-lang.org/t/linking-more-than-one-rustc-compiled-static-libraries-in-a-c-project/89778/2 [2] #259 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously to this PR, this was the state of backtrace support on panics for our supported VMs:
This adds a new configuration directive,
backtraces on, which adds support for more detailed backtraces:filenames and line numbers
The default is
backtraces off, which is the same as the old behavior (incurring in no additional initialization time for parsing the function name table for Wasmer and V8).We also replace the "semi-external dependency" cwabt (a library whose source lives in the ngx_wasm_module repository but which was not built as part of the ngx_wasm_module build process) with a properly "vendored-in" dependency ngx-wasm-rs, which implements the same functionality as a more general Rust library for ngx_wasm_module, which will allow us to add more Rust functionality over time with little deployment overhead.
The Rust library dependency is currently optional for Wasmer and mandatory for V8 (as cwabt was) — it could be made optional as well at the expense of .wat files not working.
Draft status: