Skip to content

Implement custom symbols referencing a section for machO #83

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.

Sign up for GitHub

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

Merged
m4b merged 3 commits into from
Jun 11, 2019
Merged

Implement custom symbols referencing a section for machO #83

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2cf3f44
Implement custom symbols referencing a section for machO
bjorn3 May 31, 2019
3a30fab
Rustfmt
bjorn3 May 31, 2019
fdb5d52
Add doc example for define_with_symbols
bjorn3 Jun 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions src/artifact.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use indexmap::IndexMap;
use string_interner::DefaultStringInterner;
use target_lexicon::{BinaryFormat, Triple};

use std::collections::BTreeSet;
use std::collections::{BTreeMap, BTreeSet};
use std::fs::File;
use std::io::Write;

Expand Down Expand Up @@ -68,15 +68,23 @@ pub enum ArtifactError {
/// Declaration that caused this error
new: Decl,
},
#[fail(display = "duplicate definition of symbol: {}", _0)]
#[fail(display = "Duplicate definition of symbol: {}", _0)]
/// A duplicate definition
DuplicateDefinition(String),

/// A non section declaration got custom symbols during definition.
#[fail(
display = "Attempt to add custom symbols {:?} to non section declaration {:?}",
_1, _0
)]
NonSectionCustomSymbols(DefinedDecl, BTreeMap<String, u64>),
}

#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone)]
struct InternalDefinition {
decl: DefinedDecl,
name: StringID,
symbols: BTreeMap<String, u64>,
data: Data,
}

Expand Down Expand Up @@ -130,6 +138,8 @@ pub(crate) struct Definition<'a> {
pub name: &'a str,
/// Contents of definition
pub data: &'a [u8],
/// Custom symbols referencing this section, or none for other definition types.
pub symbols: &'a BTreeMap<String, u64>,
/// Declaration of symbol
pub decl: &'a DefinedDecl,
}
Expand All @@ -141,6 +151,7 @@ impl<'a> From<(&'a InternalDefinition, &'a DefaultStringInterner)> for Definitio
.resolve(def.name)
.expect("internal definition to have name"),
data: &def.data,
symbols: &def.symbols,
decl: &def.decl,
}
}
Expand Down Expand Up @@ -349,6 +360,16 @@ impl Artifact {
/// **NB**: If you attempt to define an import, this will return an error.
/// If you attempt to define something which has not been declared, this will return an error.
pub fn define<T: AsRef<str>>(&mut self, name: T, data: Vec<u8>) -> Result<(), ArtifactError> {
self.define_with_symbols(name, data, BTreeMap::new())
}

/// Same as `define` but also allows to add custom symbols referencing a section decl.
pub fn define_with_symbols<T: AsRef<str>>(
&mut self,
name: T,
data: Vec<u8>,
symbols: BTreeMap<String, u64>,
Copy link
Owner

@m4b m4b May 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems weird to pass in a btree map; at least from the docstring wasn’t clear to me as a user how to construct this particular input; also, what are the invariants I should enforce, etc.

Note I didn’t read the implementation deeply, just try signature stuck out at me.

Could you give some motivation for why we should expose this in this manner ? :)

Copy link
Contributor Author

@bjorn3 bjorn3 May 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the easiest way :) Should I introduce a SymbolMap as wrapper?

Invariants

  • The u64 is smaller than the length of data (no out of bound symbols)
  • No symbol name (the String) is used more than once.
  • symbols is empty for non section definitions.

Copy link
Owner

@m4b m4b May 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I thought the api would be like define_in_section(name: String, data: Vec<u8>, section: String)

Why does one define a symbol, it’s data, and then provide a set of strings (section name ?) -> u64s (offsets?)

A symbol is only ever in one section, so I’m confused why the api has a collection like thing as an argument.

Copy link
Contributor Author

@bjorn3 bjorn3 May 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it the other way around: A section decl has symbols referencing to certain offsets. symbols is a map from symbol to offset within section.

Copy link
Owner

@m4b m4b Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I haven’t heard from anyone so I’m ok to merge this, but I still have a few questions.

So the symbols argument to define_with_symbols; the docs state it’s a map of custom symbols referencing a section; but where does the section come from? Is it the symbol we’re defining?

The references seem like they’re “floating” to me, relying on previous or post function calls to resolve them, is that correct ?

If yes, that worries me, because it’s essentially introduced a stateful aspect to the api; is there any way we can make the connection stronger (short of moving symbol references to interned ids (which I somewhat regret not having in the first place))

I guess the api just doesn’t seem totally clear to me; a doc example would help a lot I think

Copy link
Contributor Author

@bjorn3 bjorn3 Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it the symbol we’re defining?

Yes

The references seem like they’re “floating” to me, relying on previous or post function calls to resolve them, is that correct ?

The symbols referencing the section are newly introduced when calling this method.

I guess the api just doesn’t seem totally clear to me; a doc example would help a lot I think

Will add.

Copy link
Owner

@m4b m4b Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok got it now; yea doc example would be really great, and maybe slightly more explanation in documentation, but example I think will be best solution to my confusion/questions.

Then I think we can merge. Ping me again aggressively if I forget :)

Copy link
Contributor Author

@bjorn3 bjorn3 Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added doc example.

) -> Result<(), ArtifactError> {
let decl_name = self.strings.get_or_intern(name.as_ref());
match self.declarations.get_mut(&decl_name) {
Some(ref mut stype) => {
Expand All @@ -363,16 +384,28 @@ impl Artifact {
Err(ArtifactError::ImportDefined(name.as_ref().to_string()).into())?
}
};

match decl {
DefinedDecl::Section(_) => {}
_ => {
if !symbols.is_empty() {
return Err(ArtifactError::NonSectionCustomSymbols(decl, symbols));
}
}
}

if decl.is_global() {
self.nonlocal_definitions.insert(InternalDefinition {
name: decl_name,
data,
symbols,
decl,
});
} else {
self.local_definitions.insert(InternalDefinition {
name: decl_name,
data,
symbols,
decl,
});
}
Expand Down
15 changes: 10 additions & 5 deletions src/elf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,8 +475,10 @@ impl<'a> Elf<'a> {
}
}
}
pub fn add_definition(&mut self, name: &str, data: &'a [u8], decl: &artifact::DefinedDecl) {
let def_size = data.len();
pub fn add_definition(&mut self, def: artifact::Definition<'a>) {
let name = def.name;
let decl = def.decl;
let def_size = def.data.len();

let section_name = match decl {
DefinedDecl::Function(_) => format!(".text.{}", name),
Expand Down Expand Up @@ -519,7 +521,7 @@ impl<'a> Elf<'a> {
.align(d.get_align()),
};

let shndx = self.add_progbits(section_name, section, data);
let shndx = self.add_progbits(section_name, section, def.data);

match decl {
DefinedDecl::Function(_) | DefinedDecl::Data(_) => {
Expand All @@ -544,7 +546,10 @@ impl<'a> Elf<'a> {
}
}
DefinedDecl::Section(_) => {
// No symbols in custom sections, yet...
for (_symbol, _symbol_dst_offset) in def.symbols {
// FIXME: implement it
unimplemented!("elf: custom symbols referencing sections");
}
}
}
}
Expand Down Expand Up @@ -997,7 +1002,7 @@ pub fn to_bytes(artifact: &Artifact) -> Result<Vec<u8>, Error> {
let mut elf = Elf::new(&artifact);
for def in artifact.definitions() {
debug!("Def: {:?}", def);
elf.add_definition(def.name, def.data, def.decl);
elf.add_definition(def);
}
for (ref import, ref kind) in artifact.imports() {
debug!("Import: {:?} -> {:?}", import, kind);
Expand Down
29 changes: 27 additions & 2 deletions src/mach.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ type StrtableOffset = u64;
const CODE_SECTION_INDEX: SectionIndex = 0;
const DATA_SECTION_INDEX: SectionIndex = 1;
const CSTRING_SECTION_INDEX: SectionIndex = 2;
const NUM_DEFAULT_SECTIONS: SectionIndex = 3;

/// A builder for creating a 32/64 bit Mach-o Nlist symbol
#[derive(Debug)]
Expand Down Expand Up @@ -455,9 +456,12 @@ impl SegmentBuilder {
sections.insert(sectname.to_string(), section);
}
fn build_custom_section(
symtab: &mut SymbolTable,
sections: &mut IndexMap<String, SectionBuilder>,
offset: &mut u64,
addr: &mut u64,
symbol_offset: &mut u64,
section_idx: SectionIndex,
def: &Definition,
) {
let s = match def.decl {
Expand All @@ -483,7 +487,20 @@ impl SegmentBuilder {
flags |= S_ATTR_DEBUG;
}

for (symbol, symbol_dst_offset) in def.symbols {
symtab.insert(
symbol,
SymbolType::Defined {
section: section_idx,
segment_relative_offset: *symbol_dst_offset,
absolute_offset: *symbol_offset + *symbol_dst_offset,
global: true,
},
);
}

let local_size = def.data.len() as u64;
*symbol_offset += local_size;
let section = SectionBuilder::new(sectname, segment_name, local_size)
.offset(*offset)
.addr(*addr)
Expand Down Expand Up @@ -547,8 +564,16 @@ impl SegmentBuilder {
0,
Some(S_CSTRING_LITERALS),
);
for def in custom_sections {
Self::build_custom_section(&mut sections, &mut offset, &mut size, def);
for (idx, def) in custom_sections.iter().enumerate() {
Self::build_custom_section(
symtab,
&mut sections,
&mut offset,
&mut size,
&mut symbol_offset,
idx + NUM_DEFAULT_SECTIONS,
def,
);
}
for (ref import, _) in artifact.imports() {
symtab.insert(import, SymbolType::Undefined);
Expand Down