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

Missing API surface after move from bytemuck::Pod #294

Closed
Ben-Lichtman opened this issue May 4, 2021 · 12 comments · Fixed by #296
Closed

Missing API surface after move from bytemuck::Pod #294

Ben-Lichtman opened this issue May 4, 2021 · 12 comments · Fixed by #296

Comments

@Ben-Lichtman
Copy link

I aim to edit object files directly in memory using this library, however I've encountered the problem that the object Pod implementation is missing much of the bytemuck::Pod functionality - including the operations on mutable references which would be required for this.

I also notice that the object::pod API is also slightly different:

  • object::pod::bytes_of() = bytemuck::bytes_of()
  • object::pod::bytes_of_slice() = bytemuck::cast_slice<_, u8>()
  • object::from_bytes and object::slice_from_bytes both include a tail output alongside the transmuted output

A Move to bytemuck::Pod would break this API, however the functions that include a tail seem to only be used to implement the Bytes struct (a wrapper around a slice), and this same implementation could also be easily modified to work with bytemuck::Pod and its tail-less functions simply by subslicing the input slice before passing it into the function.

The tail-inclusive functionality could also be added into bytemuck for added ecosystem impact.

I was also able to locate this PR from a year ago #184 which doesn't seem to really justify the switch to a vendored Pod trait...

@philipc
Copy link
Contributor

philipc commented May 4, 2021

The switch to our own Pod implementation was because:

  • it allows us to avoid a dependency
  • the required implementation was trivial (especially compared to the complexity of bytemuck)
  • it allows us to provide a more convenient API (specifically the tail functionality)

I'm fine with adding mutable variants of the API if you need it. I'm unsure how this is intended to interact with the rest of the object file parsing though, which expects to hold multiple references to parts of the file (something that is harder to manage with mutable references).

@Ben-Lichtman
Copy link
Author

* the required implementation was trivial (especially compared to the complexity of bytemuck)

May I ask you to elaborate on this? Bytemuck seems pretty easy to implement to me.

@Ben-Lichtman
Copy link
Author

Ben-Lichtman commented May 4, 2021

If I implemented bytemuck for this crate in a PR would you consider merging it in? (possibly behind a feature gate to avoid a required dependency?)

As for how this would interact with parsing, I expect that the file would have to be re-parsed after each modification. The objective here is really just to leverage object's type definitions etc.

@philipc
Copy link
Contributor

philipc commented May 4, 2021

May I ask you to elaborate on this? Bytemuck seems pretty easy to implement to me.

The implementation in 2d48743 wasn't much larger than the wrappers it replaced. bytemuck handles a more general case (converting between any types, not just byte slices), which adds complexity that we don't need.

If I implemented bytemuck for this crate in a PR would you consider merging it in? (possibly behind a feature gate to avoid a required dependency?)

You'll have to convince me that it isn't enough to add from_bytes_mut and slice_from_bytes_mut to this crate.

@Ben-Lichtman
Copy link
Author

from_bytes_mut and slice_from_bytes_mut would probably satisfy requirements, and I'll happily close this issue.

@Ben-Lichtman
Copy link
Author

Oops, may also request bytes_of_mut as well?

@philipc
Copy link
Contributor

philipc commented May 4, 2021

Sure, curious what you need that for though? I expected mutation via the fields to be enough.

@Ben-Lichtman
Copy link
Author

Ben-Lichtman commented May 4, 2021

It's for screwing around and patching elf files mostly, there will likely be corruption of the fields / overlapping etc, so being able to work with raw bytes some of the time helps.

@zetanumbers
Copy link

zetanumbers commented Oct 16, 2021

Recently I have implemented a POD struct, but i've been struck with a dilemma of using object::Pod or bytemuck::Pod. First i would like to use derive(Pod) for a struct and don't think about safety conditions or specifying safety comments, it just checks everything for you. Second i would have liked to reuse object's endian-agnostic types like endian::U32Bytes<Endian>. This would be trivially easy if object have depended on bytemuck::Pod, but it isn't. After some thought i don't see any problem with object depending on bytemuck, so let me try to refute the points made for moving from bytemuck:

  • it allows us to avoid a dependency

bytemuck has zero dependencies if all except a derive feature, otherwise it pulls a standard collections of crates for derive-macros (proc-macro2, quote, syn). I don't see much of a reason to avoid having +1 dependency to this crate.

  • the required implementation was trivial (especially compared to the complexity of bytemuck)

You can ignore bytemuck's complexity except for implementing Zeroable trait for the Pod trait. Necessary complexity seems very much trivial to me.

  • it allows us to provide a more convenient API (specifically the tail functionality)

POD is POD anywhere you go. Nothing stops you from implementing current object::pod functionality, including tail functionality, on the bytemuck::Pod trait.

Also i think there's some benefit of bringing back bytemuck such as:

  • bytemuck is more complete as a common API for different crates;
  • has more std Pod types (i. e. f32 and Option<NonZeroU8>) than object::Pod has;
  • delegating responsibility of proving PODness of these types to the bytemuck crate;
  • concurrent builds (bytemuck -> object + some_pod_types) instead of (object -> some_pod_types);
  • code deduplication.

I would also like to see endian module being extracted into a different crate for similar reasons.

@zetanumbers
Copy link

All that said i would like to do the PR for any of these changes, if any of them are approved.

@philipc
Copy link
Contributor

philipc commented Oct 16, 2021

You can add an optional dependency on bytemuck if that helps, but core functionality of this crate must not depend on it.

@philipc
Copy link
Contributor

philipc commented Oct 17, 2021

@zetanumbers I see you left a confused emoji, so maybe this helps:

  • This crate is a dependency of backtrace-rs, and has restrictions on what it can depend on. So we aim to minimize dependencies for the backtrace-rs use, and it definitely can't use derives.
  • The pod support in this crate is not intended as a common API for other crates. It is intended only for use with the types provided by this crate.
  • The five points you listed are irrelevant to the goals and requirements of this crate.
  • Overall, use of bytemuck does not provide enough benefit relative to how simple it is to implement the required support in this crate, and so avoiding the dependency is the correct choice for this crate.

If you want pod support for types defined in an unrelated crate, then bytemuck is probably the correct choice, and lack of endian support in bytemuck is something that should be fixed in bytemuck or another crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants