`features = ["std", "elf32"]` doesn't build

[mkeeter]

While trimming down my dependencies, I noticed that building Goblin with default-features = false, features = ["std", "elf32"] fails:

    Checking goblin v0.6.0
error[E0432]: unresolved import `crate::elf64`
   --> /Users/mjk/.cargo/registry/src/github.com-1ecc6299db9ec823/goblin-0.6.0/src/elf/header.rs:249:17
    |
249 |             use crate::elf64;
    |                 ^^^^^^^^^^^^ no `elf64` in the root

error[E0433]: failed to resolve: could not find `elf64` in the crate root
   --> /Users/mjk/.cargo/registry/src/github.com-1ecc6299db9ec823/goblin-0.6.0/src/elf/dynamic.rs:802:35
    |
802 |     elf_dyn_std_impl!(u64, crate::elf64::program_header::ProgramHeader);
    |                                   ^^^^^ could not find `elf64` in the crate root

Some errors have detailed explanations: E0432, E0433.
For more information about an error, try `rustc --explain E0432`.

This isn't a big deal – I can add elf64 to the feature list – but the Rust Book advises against it:

A consequence of this is that features should be additive. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features.

[m4b]

Yes this is a bug. Looks like we only tested for both elfs and std https://github.com/m4b/goblin/blob/c81ebdbe5686427090726a5156bd128549ac7499/Makefile#L34 . I’m surprised this hasn’t come up before. Would you be interested in pushing a fix ? :)

[mkeeter]

I took a brief look, but it seems like there's not an obvious fix: most of the code is written assuming Container::Little / Container::Big both exist, and selecting elf32 / elf64 accordingly.

Maybe the fix is to remove the elf32 / elf64 features in favor of a single elf feature? This would obviously be a breaking change.

(I also realized that my code is using goblin::Object, which requires all features anyways, so this is a moot point for me)

[m4b]

Yea I had a feeling it was something like that :/ I wonder if we can enable both if you select either to preserve additivity? Perhaps a single elf is better as you suggest but there are some use cases only involving just one, but I think this only works without std ? Otherwise I’m not sure why there are both.

[d-e-s-o]

Also hit this issue for elf64.

[d-e-s-o]

Perhaps one possible backwards compatible fix could be:

--- Cargo.toml
+++ Cargo.toml
@@ -42,8 +42,8 @@ default = ["std", "elf32", "elf64", "mach32", "mach64", "pe32", "pe64", "te", "a
 std = ["alloc", "scroll/std"]
 alloc = ["scroll/derive", "log"]
 endian_fd = ["alloc"]
-elf32 = []
-elf64 = []
+elf32 = ["elf64"]
+elf64 = ["elf32"]
 # for now we will require mach and pe to be alloc + endian_fd
 mach32 = ["alloc", "endian_fd", "archive"]
 mach64 = ["alloc", "endian_fd", "archive"]

Given that no additional dependencies are being pulled in, just compiling a few additional code paths is probably okay and at least there is no build failure. Yes, a few "unintentional" types will be available to users, but I'd say that is outside of semver guarantees.