m4b / goblin

An impish, cross-platform binary parsing crate, written in Rust
MIT License
1.19k stars 158 forks source link

Build failures with alloc, no_std and archive features. #272

Closed dancrossnyc closed 3 years ago

dancrossnyc commented 3 years ago

When building goblin for no_std environments with the alloc and archive features enabled, I ran into build failures because the to_vec method on RelocSection in src/elf/reloc.rs uses Vec, which wasn't visible:

: chandra; cargo build --no-default-features --features=elf64,elf32,archive
   Compiling scroll v0.10.2
   Compiling goblin v0.4.0 (/Users/cross/projects/third-party/goblin)
error[E0412]: cannot find type `Vec` in this scope
   --> src/elf/reloc.rs:458:33
    |
458 |         pub fn to_vec(&self) -> Vec<Reloc> {
    |                                 ^^^ not found in this scope
    |
help: consider importing one of these items
    |
278 |     use alloc::vec::Vec;
    |
278 |     use crate::archive::Vec;
    |

warning: unused import: `crate::error`
  --> src/elf/program_header.rs:86:9
   |
86 |     use crate::error;
   |         ^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

error: aborting due to previous error; 1 warning emitted

For more information about this error, try `rustc --explain E0412`.
error: could not compile `goblin`

To learn more, run the command again with --verbose.
: chandra;

Importing alloc::vec::Vec is conditioned on the endian_fd feature being enabled, but the to_vec method (and it's use in the Debug impl on RelocSection) don't depend on endian_fd.

I was able to fix this by simply remove the #[cfg(feature = "endian_fd")] on the use alloc::vec::Vec; statement, which I believe is safe (since to_vec uses Vec and is, AFAICT, always compiled in when features include alloc):

diff --git a/src/elf/reloc.rs b/src/elf/reloc.rs
index 43f759a..6225a38 100644
--- a/src/elf/reloc.rs
+++ b/src/elf/reloc.rs
@@ -280,7 +280,6 @@ if_alloc! {
     use core::fmt;
     use core::result;
     use crate::container::{Ctx, Container};
-    #[cfg(feature = "endian_fd")]
     use alloc::vec::Vec;

     #[derive(Clone, Copy, PartialEq, Default)]

I'll send a PR.

m4b commented 3 years ago

Thanks for the fix; so i've added this line to the make api test, so it should catch these kind of regressions in future hopefully; alloc was added after all the other tests, so i think the make api test was omitted; let me know if you have any other issues with the features, especially on no std/alloc environments, this is not as well tested/covered as I would like, and would especially like to see those warnings disappear :) going to close this now since I believe you PR fixed it (and thanks again!)

m4b commented 3 years ago

I've also pushed new 0.4.1 version which includes your fix

dancrossnyc commented 3 years ago

Thank you!