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

Add 'alloc' feature #75

Closed
philipc opened this issue Jan 26, 2018 · 3 comments
Closed

Add 'alloc' feature #75

philipc opened this issue Jan 26, 2018 · 3 comments

Comments

@philipc
Copy link
Collaborator

philipc commented Jan 26, 2018

For gimli-rs/object#45, we need to be able to parse all the formats when using #[no_std]. Currently, a lot of the parsing requires allocation. Completely avoiding allocations is too much work for now, and probably not a good use of time. And anyway, we can still use the alloc crate with no_std (but this does require building with nightly currently).

So I propose we add an alloc feature that is midway between no_std and std. This feature will cover everything that uses allocations, and so mostly what will be left in std will be things that use std::fs or std::io.

I've done enough to verify that this approach works, so assuming this is acceptable, now I just need to clean it up and submit a PR.

One question I have is about the endian_fd feature. What exactly is this meant to cover? The readme says it 'parses according to the endianness in the binary', but it doesn't cover code such as
https://github.com/m4b/goblin/blob/master/src/elf/section_header.rs#L490. Currently it gates the entire mach/pe/archive formats, and it also requires 'std'. I want to relax this restriction as part of adding the alloc feature, but I'm not sure which parts of mach/pe/archive should still require it.

@m4b
Copy link
Owner

m4b commented Jan 28, 2018

I realized I started typing up bunch of stuff, but seems like you've done most of this, just wanted clarification on some things hehe.

Sorry for the delay;

So for endian_fd - after a while, the approach I took for the "unified", scroll-based, endian-aware structs was to place the definitions under the std flag, as they are useful to non-parser downstream usages, e.g., hattps://github.com/m4b/faerie uses the unified structs for writing in a cross-platform/cross-endian way., but has no use for the parser infrastructure. e.g., goblin::Object

That being said, the endian_fd/std flags have lost semantic coherence somewhat over time, imho, and might be nice to figure out exactly what the distinction is.

For example, I initially imagined the following hierachy:

  1. core - no alloc
  2. std - alloc + io::Read/Write
  3. endian_fd - an endian aware parser implementation

Honestly, at this point, endian_fd should just be parser, because it is the parse methods + the Object infra.

The parser flag should also turn on every other flag, since its the default + the maximal api surface.

As for other mach/pe feature gates, I think they're all alloc (using your terminology) + endian_fd for the parse methods/top level objects, like mach::MachO

It's hard to say; if you have a PR might be better to open up and i'll be able to look at the diff, or where your thoughts are/what you're thinking, before it gets to be too much of a monster?

@philipc
Copy link
Collaborator Author

philipc commented Jan 29, 2018

Honestly, at this point, endian_fd should just be parser, because it is the parse methods + the Object infra.

Ok that makes sense. Do you count the scroll traits as part of the parser?

The parser flag should also turn on every other flag, since its the default + the maximal api surface.

I'm not sure about that. I think it should be possible to enable parsing for only the bits you need.

So I think the hierarchy should be:

  1. core
  2. core + alloc
  3. std

The parser flag and the file formats flags should be orthogonal to that, but it's fine if they have a minimum required level (eg parsers will require at least alloc). For the object crate, the change I need here is to lower the minimum required level for parsers so that they only require alloc instead of std.

@philipc
Copy link
Collaborator Author

philipc commented Jan 29, 2018

The work I've done so far is just to verify that I can do parsing in the object crate without depending on std, but the changes I've done for that are just hacks to get it to compile. So if the above kind of sounds like the right way to go, then I'll clean it up to look like that, then can review further.

@m4b m4b closed this as completed in #77 Feb 7, 2018
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

No branches or pull requests

2 participants