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

Fix clippy lints #2942

Merged
merged 1 commit into from
Jun 9, 2022
Merged

Fix clippy lints #2942

merged 1 commit into from
Jun 9, 2022

Conversation

silwol
Copy link
Contributor

@silwol silwol commented Jun 9, 2022 •
edited
Loading

Fixes: #2926

Description

  • Fix all clippy lints
  • Adjust Makefile so that the cargo clippy commands exit with non-zero when warnings are found

TODO before this is ready

  • Describe safety in lib/engine/src/tunables.rs:67
  • Describe safety in lib/types/src/values.rs:70
  • Describe safety in lib/types/src/values.rs:76
  • Describe safety in lib/emscripten/src/utils.rs:126
    I don't know why it needs to be unsafe, can you research why?
    • The reason is the call to unsafe slice::from_raw_parts_mut(addr, count as usize) in line 135. Not sure if this function actually encapsulates the behavior good enough, but if it does, we could just put a unsafe{…} around this call and remove the unsafe from the function. (silwol)
  • Describe safety in lib/emscripten/src/utils.rs:141
    We need to double check the safety of the previous method, as this one depends on it (and I believe that's why it's unsafe)
    • That appears to be the case, once the previous method is no longer marked unsafe, the unsafe here is no longer required either. (silwol)
  • Describe safety in lib/c-api/src/wasm_c_api/wat.rs:13
    Not sure why it's unsafe, can you research?
    • Ok, extern "C", that's why. No idea why I missed that initially. :-) (silwol)
  • Describe safety in lib/api/src/sys/externals/function.rs:866
    Not sure why it's unsafe, can you research?
    • I have no clear idea, just the gut feeling that it has something to do with handling the native types. Really would appreciate if we had a better description than what I could guess. (silwol)

Review

  • Add a short description of the change to the CHANGELOG.md file

@silwol silwol force-pushed the dev/2926-fix-clippy-warnings-in-wasmer3 branch 4 times, most recently from 3503fc9 to bd90bfc Compare June 9, 2022 10:54
@syrusakbary
Copy link
Member

Describe safety in lib/emscripten/src/utils.rs:126

# Safety
This method is unsafe because it operates directly with the slice of memory represented by the address

Describe safety in lib/emscripten/src/utils.rs:141

# Safety
This method is unsafe because it uses `allocate_on_stack` which is unsafe

Describe safety in lib/api/src/sys/externals/function.rs:866

# Safety
This trait is unsafe given the nature of how values are written and read from the native stack

@silwol silwol force-pushed the dev/2926-fix-clippy-warnings-in-wasmer3 branch from bd90bfc to b80466d Compare June 9, 2022 11:09
@silwol silwol marked this pull request as ready for review June 9, 2022 11:11
@silwol silwol requested a review from syrusakbary as a code owner June 9, 2022 11:11
@silwol silwol force-pushed the dev/2926-fix-clippy-warnings-in-wasmer3 branch 2 times, most recently from 1537dea to 6aad82c Compare June 9, 2022 13:44
@silwol silwol force-pushed the dev/2926-fix-clippy-warnings-in-wasmer3 branch from 2ebb905 to 5339f7c Compare June 9, 2022 14:06
@syrusakbary
Copy link
Member

bors r+

@syrusakbary syrusakbary merged commit 66c1c00 into wasmerio:master Jun 9, 2022
bors bot added a commit that referenced this pull request Jun 9, 2022
@bors @silwol
Merge #2942
60c9d1b 
2942: Fix clippy lints r=syrusakbary a=silwol

Fixes: #2926

# Description
* Fix all clippy lints
* Adjust `Makefile` so that the `cargo clippy` commands exit with non-zero when warnings are found

# TODO before this is ready
- [x] Describe safety in [lib/engine/src/tunables.rs:67](https://github.com/silwol/wasmer/blob/dev/2926-fix-clippy-warnings-in-wasmer3/lib/engine/src/tunables.rs#L67)
- [x] Describe safety in [lib/types/src/values.rs:70](https://github.com/silwol/wasmer/blob/dev/2926-fix-clippy-warnings-in-wasmer3/lib/types/src/values.rs#L70)
- [x] Describe safety in [lib/types/src/values.rs:76](https://github.com/silwol/wasmer/blob/dev/2926-fix-clippy-warnings-in-wasmer3/lib/types/src/values.rs#L76)
- [x] Describe safety in [lib/emscripten/src/utils.rs:126](https://github.com/silwol/wasmer/blob/dev/2926-fix-clippy-warnings-in-wasmer3/lib/emscripten/src/utils.rs#L126)
I don't know why it needs to be unsafe, can you research why?
  - The reason is the call to `unsafe slice::from_raw_parts_mut(addr, count as usize)` in line 135. Not sure if this function actually encapsulates the behavior good enough, but if it does, we could just put a `unsafe{…}` around this call and remove the `unsafe` from the function. (silwol)
- [x] Describe safety in [lib/emscripten/src/utils.rs:141](https://github.com/silwol/wasmer/blob/dev/2926-fix-clippy-warnings-in-wasmer3/lib/emscripten/src/utils.rs#L141)
We need to double check the safety of the previous method, as this one depends on it (and I believe that's why it's unsafe)
  - That appears to be the case, once the previous method is no longer marked unsafe, the `unsafe` here is no longer required either. (silwol)
- [x] Describe safety in [lib/c-api/src/wasm_c_api/wat.rs:13](https://github.com/silwol/wasmer/blob/dev/2926-fix-clippy-warnings-in-wasmer3/lib/c-api/src/wasm_c_api/wat.rs#L13)
Not sure why it's unsafe, can you research?
  - Ok, `extern "C"`, that's why. No idea why I missed that initially. :-) (silwol)
- [x] Describe safety in [lib/api/src/sys/externals/function.rs:866](https://github.com/silwol/wasmer/blob/dev/2926-fix-clippy-warnings-in-wasmer3/lib/api/src/sys/externals/function.rs#L866)
Not sure why it's unsafe, can you research?
  - I have no clear idea, just the gut feeling that it has something to do with handling the native types. Really would appreciate if we had a better description than what I could guess. (silwol)

# Review

- [x] Add a short description of the change to the CHANGELOG.md file

Co-authored-by: Wolfgang Silbermayr <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 9, 2022

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

@silwol silwol deleted the dev/2926-fix-clippy-warnings-in-wasmer3 branch June 9, 2022 16:02
epilys added a commit that referenced this pull request Jun 13, 2022
@epilys
Fix regression on gen_import_call_trampoline_arm64()
15ff6b9 
`Fix clippy lints #2942` changed the
`gen_import_call_trampoline_arm64()` code because clippy emitted a lint
about the loop logic. However, the fix was wrong and was causing
incorrect binary generation on arm64.

More precisely, this was caught by test
serialize::test_deserialize::singlepass::* on macos M1 (arm64)
@epilys epilys mentioned this pull request Jun 13, 2022
Merged
epilys added a commit that referenced this pull request Jun 13, 2022
@epilys
Fix regression on gen_import_call_trampoline_arm64()
485dde5 
Fix clippy lints #2942 changed the `gen_import_call_trampoline_arm64()`
code because clippy emitted a lint about the loop logic. However, the
fix was wrong and was causing incorrect binary generation on arm64.

More specifically, this was caught by test
`serialize::test_deserialize::singlepass::*` on macos M1 (arm64)
bors bot added a commit that referenced this pull request Jun 13, 2022
@bors @epilys
Merge #2948
b0d3818 
2948: Fix regression on gen_import_call_trampoline_arm64() r=epilys a=epilys

Fix clippy lints #2942  changed the `gen_import_call_trampoline_arm64()` code because clippy emitted a lint about the loop logic. However, the fix was wrong and was causing incorrect binary generation on arm64.

More precisely, this was caught by test `serialize::test_deserialize::singlepass::*` on macos M1 (arm64)


This regression was not caught because **tests are not run on ARM64 on Github Actions CI runners**.

Co-authored-by: Manos Pitsidianakis <[email protected]>
ptitSeb pushed a commit that referenced this pull request Oct 20, 2022
@epilys @ptitSeb
Fix regression on gen_import_call_trampoline_arm64()
c3bbee7 
Fix clippy lints #2942 changed the `gen_import_call_trampoline_arm64()`
code because clippy emitted a lint about the loop logic. However, the
fix was wrong and was causing incorrect binary generation on arm64.

More specifically, this was caught by test
`serialize::test_deserialize::singlepass::*` on macos M1 (arm64)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epilys epilys epilys approved these changes

@syrusakbary syrusakbary syrusakbary approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix Clippy warnings in Wasmer 3
3 participants
@silwol @syrusakbary @epilys