Stream: git-wasmtime

Topic: wasmtime / PR #9492 test-macros: Handle dead code warnings

Wasmtime GitHub notifications bot (Oct 21 2024 at 18:27):

saulecabrera opened PR #9492 from saulecabrera:test-macros-handle-dead-code-warnings to bytecodealliance:main:

This commit handles dead code warnings produced by the compiler when the macro is invoked as:

#[wasmtime_test(strategies(not(Cranelift))]

Which occur because Winch currently only offers support for x86_64.

The motivation behind this change comes from
https://github.com/bytecodealliance/wasmtime/pull/9490, which is where the warnings surfaced.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Oct 21 2024 at 18:27):

saulecabrera requested elliottt for a review on PR #9492.

Wasmtime GitHub notifications bot (Oct 21 2024 at 18:27):

saulecabrera requested wasmtime-core-reviewers for a review on PR #9492.

Wasmtime GitHub notifications bot (Oct 21 2024 at 18:27):

saulecabrera requested alexcrichton for a review on PR #9492.

Wasmtime GitHub notifications bot (Oct 22 2024 at 14:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 22 2024 at 14:48):

alexcrichton created PR review comment:

As an alternative instead of #[cfg(target_arch = "x86_64")] being emitted, how about #[cfg_attr(target_arch = "x86_64", ignore)]? That way we wouldn't have to worry about unused warnings or dead code I think

Wasmtime GitHub notifications bot (Oct 22 2024 at 15:25):

saulecabrera created PR review comment:

I briefly tried this approach, I didn't continue down this route because the compiler was complaining about multiple cfg predicates are specified, presumably because some tests have other extra attributes like #[cfg_attr(miri, ignore)] -- I think we could parse those attributes and modify them, however, since this is "temporary" (i.e., once we can fully test aarch64) I thought it might make sense to go with this approach.

Wasmtime GitHub notifications bot (Oct 22 2024 at 15:25):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 22 2024 at 15:27):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 22 2024 at 15:27):

alexcrichton created PR review comment:

Ah ok makes sense yeah. I wonder though, could we take a leaf out of the book of tests/wast.rs and do #[cfg_attr(..., should_fail)] instead? That way we're guaranteed to run the test and we just assert that it's an error instead of passing.

Wasmtime GitHub notifications bot (Oct 22 2024 at 17:06):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 22 2024 at 17:06):

saulecabrera created PR review comment:

Had an offline discussion and concluded that should_panic probably won't play nicely with tests that return Result, so we're moving forward with the current approach.

Wasmtime GitHub notifications bot (Oct 22 2024 at 17:21):

saulecabrera merged PR #9492.

Last updated: Nov 22 2024 at 17:03 UTC