-
Notifications
You must be signed in to change notification settings - Fork 163
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 #77
Conversation
This allows building the `endian_fd` feature without `std`, although it does still require the `alloc` crate and a nightly rustc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So overall this looks good, and a lot of work, thank you!
So, I don’t think it’s true, but just to be clear, this does not now mean goblin will only compile on nightly, right ?
Can use on stable unless you explicitly request the alloc feature ? Then required to use nightly, yes ?
If this forces nightly unconditionally I can’t merge these changes, but I don’t think it does.
However the feature flag switching compiler requirements does worries me a little since features are supposed to be additive otherwise it can break cargo and have weird effects at a distance, or so I’m told.
Could you clarify a bit ?
Also if anyone else is wants to chime in, now is a good time
use error; | ||
use core::fmt; | ||
use core::result; | ||
use core::ops::Range; | ||
use scroll::ctx; | ||
use container::{Container, Ctx}; | ||
|
||
#[cfg(feature = "endian_fd")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be endian fd or alloc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endian_fd
, because it's only used by SectionHeader::parse
.
use std::io::{Read, Seek}; | ||
use std::io::SeekFrom::Start; | ||
if_std! { | ||
use error::Result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is error in std but not alloc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's only used by from_fd
, which is only enabled for std
.
@@ -334,9 +335,9 @@ pub struct Archive<'a> { | |||
sysv_name_index: NameIndex<'a>, | |||
// the array of members, which are indexed by the members hash and symbol index | |||
member_array: Vec<Member<'a>>, | |||
members: HashMap<&'a str, usize>, | |||
members: BTreeMap<&'a str, usize>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this change? Because of random requirements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because HashMap
depends on std
, but BTreeMap
is available in alloc
. An alternative is to try to split up the archive support so that the HashMap
stuff is only enabled for std
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh it’s fine. I don’t think O(n) lookup important for members, since there’s usually only like 10
extern crate scroll; | ||
|
||
#[cfg_attr(feature = "std", macro_use)] | ||
#[cfg(feature = "std")] extern crate log; | ||
#[cfg(feature = "log")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log isn’t a feature, will this work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, cargo adds features for optional dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that’s awesome, did not know this!
|
||
pub mod strtab; | ||
#[cfg(feature = "std")] | ||
mod alloc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a mod?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevremind, it’s getting reexportrd and used elsewhere
@@ -58,8 +60,7 @@ impl FatHeader { | |||
} | |||
|
|||
#[repr(C)] | |||
#[derive(Clone, Copy, Default)] | |||
#[cfg_attr(feature = "std", derive(Pread, Pwrite, SizeWith))] | |||
#[derive(Clone, Copy, Default, Pread, Pwrite, SizeWith)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my understanding is correct these are unconditionally derived because in order for the module to be compiled it must have mach and alloc feature, yes? In which case we are safe to derive them.
If so I almost prefer the explicit cfg feature since although it’s redundant it gives reader a clue what’s going on. Or in case of refactors it’s a second fail safe of sorts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, mach
depends on alloc
, and alloc
depends on scroll/derive
. I kind of agree with you, except that there's a ton of these in load_command.rs
and I was lazy. Additionally, for the ones in load_command.rs
we would also need cfg_attr(feature = "std", derive(IOread, IOwrite))
(I don't actually understand why they don't give errors currently...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea be lazy, I forgot there’s like 40 of them ;)
|
||
#[cfg(all(feature = "endian_fd", feature = "elf64", feature = "elf32", feature = "pe64", feature = "pe32", feature = "mach64", feature = "mach32", feature = "archive"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it safe to remove the intersection of all these features here and in the impl ?
Couldn’t the user compile with elf, mach, and std and the get a missing symbol error in the impl parser since PE wasn’t compiled.
Or does endian_fd mean everything is enabled now ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #75 (comment), you said that the parser flag (aka endian_fd) should turn on every other flag. So that is now enforced in Cargo.toml. Since this is all wrapped in if_endian_fd
, specifying the features here is redundant. As I said in that issue, I'm not sure if this is correct, but I actually kind of like not needing to list out all the features here, so I went with that, but I'm happy to change it.
Yes, you can use on stable as long as either you request |
The features are still additive, so I don't think it will break cargo. I think it's fine to have a feature that depends on nightly, plenty of crates do that, and this case isn't even as bad as some other uses of nightly because you can always request std in order to avoid the nightly requirement. |
So I should have asked this before but all things were done in haste, but what is the general motivating usecase for having the alloc flag? So originally I thought that object wanted a no_std parser because it was being considered/suggested for use in the rustc compiler in its backtrace mechanism. rust-lang/rust#44251 The first thing is I don’t understand precisely why the rustc compiler would require no std there, and furthermore why we couldn’t allocate. Perhaps that explains why the alloc flag is just for allocation parts? But I still don’t understand who the alloc flag is for, specifically its nightly requirement, etc. So essentially I’m a little worried if the changes here are a solution in need of a problem. |
I'm not clear on that either, since rust-lang/rust#44251 is modifying libstd and using std::io. @whitequark can you elaborate?
But we can allocate. If we couldn't allocate then there would be no point adding an alloc feature.
The purpose of the alloc flag is for the parts that require allocation, but don't require libstd. The nightly requirement is because the alloc crate is not stable. |
I’ll address couple comments in a bit but also why are the tests failing ? |
Tests are failing on rust 1.18 because of cyclic features, which is only supported in relatively recent rust. The fix for that depends on what we decide about the endian_fd feature changes that you commented on. |
I just meant in the context of the compiler, it seemed like no_std was required, unless I missed something, and I was trying to figure out why that would be required. Is it correct to say that in order to print backtraces in rustc, And i'm sure the C libbacktrace lib allocates, and is a regular C/C++ library without any funny business.
So my point in asking about who wants it was to try to get a feel for whether it's actually a real pressing usecase. Specifically, I understand at this point how it works, but I'm trying to understand some usecases for it. Which brings me to @whitequark :
Ok, so this is start for a good motivating example. If I'm understanding right, on or1k firmware you're in a I'm still curious also how goblin/object would help you in terms of "not requiring the user to manually run addr2line" - are you saying that object would be embedded in the firmware itself, and when an exception is raised of some kind, it prints the backtrace automatically before termination? (if so, that's awesome) Bringing it back to the original rustc pr for backtrace printing, @whitequark, are the changes here, which allow a Sorry for all the questions, just trying to understand the precise problem that's being solved by adding |
I'm not familiar with cyclic features; do you know offhand which rust? I would really like to avoid a rustc version bump if possible, and am ok with a little momentary pain/redundancy if it means not bumping, but it depends on what exactly needs to change in order for that to happen. |
The cyclic features PR is rust-lang/cargo#4473, I don't which rust that is in. But my intention is to fix the cycle, so that shouldn't require a version bump. However, I don't want to spend effort on that until we decide which way the endian_fd dependency should be. Either way can work, what's in this PR is just my best effort based on my understanding of your comments in #75, but maybe I misunderstood. |
It uses anonymous mmap to allocate memory instead of malloc/free. |
So it has a posix libc... with an operating system that allocates memory for you? So not no_std at all |
So I'm not sure what we're deciding here, but it sounds like its just whether endian_fd means the union of every feature, i.e., I don't think its unreasonable to want just an Archive or just ELF or just Mach-o parser (and the crate was built this way), so I think its better to leave this. If you want the Object parser, then you need all of the features enabled (the default); like it is now. It sounds like the changes as they are now would force you to have the Object parser (and hence the archive, Elf, and PE parsers), even if you just wanted the Mach parser, yes? I think keeping the way it is now, with composable parsers, and a |
That is correct. Of course, goblin would be better off as a piece of software in general if it didn't allocate, and goblin not allocating would let me print backtraces in case of out-of-memory errors (we have a 4MB RAM region currently dedicated to that CPU core), but goblin+alloc would still be a huge improvement.
Correct. I already have an ELF loader for other reasons, so I would simply flash a statically linked ELF with debug symbols. |
Agreed. I've reverted the endian_fd dependency changes, and tests pass now. |
Ok this is looking good to me. I will test this later, will likely merge over weekend. If anyone has any comments speak now or forever hold your peace. This is pretty significant change. Thanks for your hard work and guidance on this @philipc! /cc @willglynn if you have any comments (and again anyone else) |
This is looking, good, doesn't appear to be a breaking change at all! Just tried this with dryad, which somehow miraculously had no other breaking changes, and works fine (besides segfaulting as expected) So, one question I have is: what happens when user specifies alloc + std in feature flags, std is used instead right? |
Yes, std already depends on alloc in Cargo.toml. |
Alright let's merge this fella |
Minor nit, I'll fix it later, but shouldn't:
just be:
since |
Doesn't matter to me, but I think it documents the dependency better: there's non-endian_fd code in mach that depends on alloc. If we later changed mach so that it didn't depend on endian_fd, there would still be code in mach that depended on alloc. |
ya, agreed actually. |
This allows building the
endian_fd
feature withoutstd
, although it does still require thealloc
crate and a nightly rustc.Fixes #75