Skip to content

Add the possibility to have two files in entry and then merge their info#91

Merged
calixteman merged 5 commits intomozilla:masterfrom
calixteman:elf-2
Jun 9, 2020
Merged

Add the possibility to have two files in entry and then merge their info#91
calixteman merged 5 commits intomozilla:masterfrom
calixteman:elf-2

Conversation

@calixteman
Copy link
Copy Markdown
Collaborator

No description provided.

@calixteman calixteman requested a review from gabrielesvelto May 27, 2020 10:03
Copy link
Copy Markdown
Collaborator

@gabrielesvelto gabrielesvelto left a comment

Choose a reason for hiding this comment

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

This seems OK overall but I feel there's some unrelated changes which I'd like to see split into another patch if possible.

Comment thread Cargo.toml Outdated
Comment thread src/action.rs
Comment on lines +98 to +101
buf_1: Vec<u8>,
filename_1: String,
buf_2: Vec<u8>,
filename_2: String,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If there's an order to those files it'd be better to give them more meaningful names such as buffer_stripped and buffer_debuginfo. Alternatively it would be better to use an array which might be useful when symbols are stripped across more than two files.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, we can do that in an other commit

Comment thread src/linux/elf.rs Outdated
})
}

pub fn merge(left: ElfInfo, right: ElfInfo) -> ElfInfo {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I must admit I'm a bit stumped by this. I thought that symbolic would already have functionality to present a single view of a PE+PDB or .so+.dbg couple. It's a bit sad to see we have to merge the debuginfo ourselves.

Comment thread src/linux/elf.rs
Comment thread src/linux/elf.rs
Comment on lines -181 to -194

/// Set the location where the inlinees are inlined in the inliner
/// We use this function when some addresses in an inlinee are before the function start
fn set_line_info(&mut self, line: &LineInfo, compilation_dir: &[u8], source: &mut SourceFiles) {
if let Some(inlinee_pos) = self.addresses.get(&line.address) {
let inlinee = &mut self.inlinees[*inlinee_pos];
if inlinee.start == line.address {
let file_id = source.get_id(compilation_dir, &line.file);
inlinee.location = ElfLineInfo {
file_id,
line: line.line as u32,
};
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This and the other changes to the inlined functions seem unrelated to the multi-file support. Is this the functionality that was recently merged into symbolic? If it is it would be best to split it out into another patch for clarity.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I moved that stuff in a separate commit.

@calixteman calixteman merged commit 5b62b84 into mozilla:master Jun 9, 2020
@calixteman calixteman deleted the elf-2 branch June 9, 2020 07:57
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

Successfully merging this pull request may close these issues.

2 participants