Stream: git-wasmtime

Topic: wasmtime / PR #8041 Optimize aarch64 relocation code by e...


view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 02:03):

glaziermag opened PR #8041 from glaziermag:local_branch to bytecodealliance:main:

Hello, beginner here, impassioned by the project. This pull request addresses issue #7588, which highlights the prevalent use of unsafe pointer offsets in the aarch64 relocation code within the cranelift-jit component. The current approach frequently casts these unsafe offsets to usize or isize types, suggesting an opportunity to improve code safety and readability by performing these casts eagerly. Please feel free to correct any noob mistakes. I'm eager to learn and contribute more effectively!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 02:03):

glaziermag requested wasmtime-compiler-reviewers for a review on PR #8041.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 02:03):

glaziermag requested elliottt for a review on PR #8041.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 02:03):

glaziermag requested wasmtime-default-reviewers for a review on PR #8041.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 02:12):

glaziermag updated PR #8041.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 08:45):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 08:45):

bjorn3 created PR review comment:

Please revert this commit. Dependency updates require reviewing the changes in those dependencies by a trusted reviewer, which cargo-vet enforces. Because no such review has been done, CI would fail once it runs.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 08:48):

bjorn3 created PR review comment:

This would loose optimizations and isn't more correct. .offset() is unsafe as it is UB to go out of bounds, but if the calculation goes out of bounds, this crate would produce UB anyway, except now sanitizers no longer know that going out of bounds is invalid and thus wouldn't trigger at the root cause of the UB.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 08:48):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 10:40):

glaziermag updated PR #8041.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 10:50):

glaziermag closed without merge PR #8041.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2024 at 10:50):

glaziermag commented on PR #8041:

Thank you for your insights, @bjorn3. Based on your feedback, I understand the concerns regarding optimization and safety. I'll proceed to close this pull request.


Last updated: Oct 23 2024 at 20:03 UTC