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.

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

offset_of! ecosystem testing #111839

Closed
14 of 16 tasks
est31 opened this issue May 22, 2023 · 17 comments
Closed
14 of 16 tasks

offset_of! ecosystem testing #111839

est31 opened this issue May 22, 2023 · 17 comments
Labels
E-help-wanted Call for participation: Help is requested to fix this issue. F-offset_of `#![feature(offset_of)]`

Comments

@est31
Copy link
Member

est31 commented May 22, 2023

I'm filing this issue to get some testing of the offset_of macro, whose implementation has recently been merged in #106934 .

There is already widespread use of the offset_of macro in the ecosystem through an implementation supporting stable via the memoffset crate, whose version 0.9.0 supports the unstable_offset_of feature due to Gilnaa/memoffset#72. So for a lot of the users, testing out is as simple as updating memoffset and running the tests with the unstable_offset_of feature enabled. Thankfully, for testing one doesn't have to add that feature to Cargo.toml, creating a requirement on nightly, one can just add --features memoffset/unstable_offset_of and it will work.

The biggest reverse dependencies of memoffset are:

It would be good to go through this list, and:

  1. update the memoffset crate to 0.9.0, ideally filing a PR (but not enabling unstable_offset_of in Cargo.toml unless the project already requires nightly!)
  2. run cargo +nightly test --features memoffset/unstable_offset_of or cargo +nightly run --features memoffset/unstable_offset_of etc, trying out the feature and seeing if something is broken, ideally also the --all parameter if it's a workspace
  3. tell here about the experience

In addition of that, it is useful to get feedback from any project that uses the memoffset crate indirectly as a transitive dependency. For that, one also has to wait for the upgrade to 0.9.0 to go through the dependency chain and then one can add a direct (unused) dependency on memoffset to enable the feature, and test it out.

For example, field-offset has a large number of reverse dependencies that one could go through also. I have made a PR for rustc in #112298.

cc #106655 tracking issue

@est31 est31 changed the title offset_of ecosystem testing offset_of! ecosystem testing May 22, 2023
@est31
Copy link
Member Author

est31 commented May 22, 2023

@rustbot label F-offset_of

@rustbot rustbot added the F-offset_of `#![feature(offset_of)]` label May 22, 2023
@est31
Copy link
Member Author

est31 commented May 22, 2023

@rustbot label E-help-wanted

@rustbot rustbot added the E-help-wanted Call for participation: Help is requested to fix this issue. label May 22, 2023
bors bot added a commit to crossbeam-rs/crossbeam that referenced this issue May 30, 2023
981: epoch: bump `memoffset` to v0.9 r=taiki-e a=Jules-Bertholet

CC: rust-lang/rust#111839

Co-authored-by: Jules Bertholet <[email protected]>
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 10, 2023
…Simulacrum

Update field-offset and enable unstable_offset_of

This makes the compiler use the builtin `offset_of!()` macro, through the wrappers in memoffset and then in field-offset.

cc  rust-lang#111839
sunfishcode added a commit to bytecodealliance/rustix that referenced this issue Jun 11, 2023
Per rust-lang/rust#111839, I've also run the tests with
`--features memoffset/unstable_offset_of`, with both the linux-raw and
libc backends, and they passed.
@sunfishcode
Copy link
Member

With bytecodealliance/rustix#689 I've now updated rustix to memoffset 0.9, and run the tests with unstable_offset_of, and everything passed.

sunfishcode added a commit to bytecodealliance/rustix that referenced this issue Jun 11, 2023
Per rust-lang/rust#111839, I've also run the tests with
`--features memoffset/unstable_offset_of`, with both the linux-raw and
libc backends, and they passed.
@sunfishcode
Copy link
Member

With bytecodealliance/wasmtime#6731, wasmtime-runtime is now updated to memoffset 0.9, and I've now run the tests with unstable_offset_of, and everything passed.

@sanbox-irl
Copy link

has been merged to main in imgui-rs and passes just fine!

@joshlf
Copy link
Contributor

joshlf commented Oct 23, 2023

Looks like PRs for pyo3 and field-offset have merged.

joshlf added a commit to joshlf/wasmer that referenced this issue Oct 23, 2023
@joshlf
Copy link
Contributor

joshlf commented Oct 23, 2023

PR for wasmer-types/wasmer-vm: wasmerio/wasmer#4273

joshlf added a commit to joshlf/glium that referenced this issue Oct 23, 2023
@joshlf
Copy link
Contributor

joshlf commented Oct 23, 2023

PR for glium: glium/glium#2081

Testing results: glium/glium#2081 (comment)

@joshlf
Copy link
Contributor

joshlf commented Oct 23, 2023

Looks like virtio-queue is already done: rust-vmm/vm-virtio@f69ec8a

joshlf added a commit to joshlf/ggez that referenced this issue Oct 23, 2023
@joshlf
Copy link
Contributor

joshlf commented Oct 23, 2023

PR for ggez: ggez/ggez#1259

Looks like ggez is already done on their devel branch: ggez/ggez@0afcab2

joshlf added a commit to joshlf/wasmer that referenced this issue Oct 23, 2023
@ojeda
Copy link
Contributor

ojeda commented Oct 23, 2023

For what it is worth, we have been successfully using it in the kernel for a while, and the first mainline user should be landing soon: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git/commit/?h=for-6.7-rust-bindings&id=7324b88975c525a013ae0db747df97924ce80675.

@est31
Copy link
Member Author

est31 commented Oct 23, 2023

@joshlf thanks for filing these pull requests. For checking the checkboxes, can you verify that enabling the offset_of mode of memoffset works in these repos? As in, the test suite runs successfully with the feature enabled (see point 2 on how to do it).

@joshlf
Copy link
Contributor

joshlf commented Oct 24, 2023

Confirmed for field-offset. Confirmed for pyo3 (there were trybuild failures, but I believe they're related to compiler version, not behavior changes).

Some of the remaining crates are difficult to test locally for one reason or another. I've put up do-not-merge PRs that enable memoffset's unstable_offset_of feature so that CI can run on those PRs as a replacement for running the test suites locally.

rust-vmm/vm-virtio#268 (CI only tests on stable; I've requested a maintainer to run for us: rust-vmm/vm-virtio#269)
ggez/ggez#1260

@joshlf
Copy link
Contributor

joshlf commented Oct 24, 2023

Confirmed for virtio-queue.

@joshlf
Copy link
Contributor

joshlf commented Oct 25, 2023

@est31 Are we hoping to get to 100% coverage here, or is 14/16 good enough? (For the remaining 2, we haven't found issues, we've just run into unrelated problems running the test or getting the feature merged.)

@est31
Copy link
Member Author

est31 commented Oct 28, 2023

It's not a precise science, my main goal for the list was to give a starting point. I think we can close this particular issue which was mostly to get things started, as projects often use stable only and are afraid to use nightly features (and indeed there is discussions to maybe change the syntax of offset_of). Of course, even if this issue is closed, further testing will always be encouraged.

Thanks ❤️ @joshlf for your help, and thanks @ojeda and everyone else for confirming that it works for their use cases.

@est31 est31 closed this as completed Oct 28, 2023
@ojeda
Copy link
Contributor

ojeda commented Oct 28, 2023

My pleasure!

theduke pushed a commit to joshlf/wasmer that referenced this issue Nov 2, 2023
theduke pushed a commit to wasmerio/wasmer that referenced this issue Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-help-wanted Call for participation: Help is requested to fix this issue. F-offset_of `#![feature(offset_of)]`
Projects
None yet
Development

No branches or pull requests

6 participants