Skip to content
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

WIP: Implement symbolication precognition #1

Closed
wants to merge 8 commits into from
Closed

WIP: Implement symbolication precognition #1

wants to merge 8 commits into from

Conversation

vvuk
Copy link

@vvuk vvuk commented May 14, 2024

WIP horrible, but maybe not totally horrible?

Notes:

  • the precog string table is not used right now, to make it easier to inspect the data. All strings are just placed as strings.
  • GlobalLibTable gained a parallel used_rvas array because I didn't want to modify the LibraryInfo type (lots of cascading effects)
  • I had to remove Clone from wholesym::SymbolManagerConfig. Keeping it was causing me all sorts of pain with the dyn trait object.
  • The &'static for the PrecogHelper is also a hack, but it might be one that's in the "whatever" category
  • If there is a precog helper, this PR searches only it for symbols, for testing (removed in followup commit but can be uncommented)

@vvuk vvuk force-pushed the precog branch 2 times, most recently from 3aa3054 to 7ac737f Compare May 14, 2024 23:27
Copy link

@mstange mstange 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 pretty reasonable! Somewhat unfortunate to have to expose the SymbolMapTrait through two or three layers of crates, but for a temporary hack, it seems acceptable.


#[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq, Hash)]
//struct StringTableIndex(usize);
struct StringTableIndex(String);
Copy link

Choose a reason for hiding this comment

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

Nice index :P

Comment on lines 221 to 222
unsafe impl Sync for PrecogSymbolInfo {}
unsafe impl Send for PrecogSymbolInfo {}
Copy link

Choose a reason for hiding this comment

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

Don't think these are needed

&self,
info: &LibraryInfo,
) -> Option<(Self::FL, Box<dyn SymbolMapTrait + Send + Sync>)> {
if let Some(precog_helper) = self.config.precog_helper.as_ref() {
Copy link

Choose a reason for hiding this comment

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

I would just have a HashMap<(String, DebugId), Arc<dyn SymbolMapTrait + Send + Sync>> stored inside the helper. And then you would add those with wholesym::SymbolManager::add_precog_symbol_map similarly to how we add known libs. This wouldn't require an extra trait.

@@ -11,6 +11,7 @@ readme = "README.md"

[dependencies]

samply-symbols = { version = "0.22.0", path = "../samply-symbols" }
Copy link

Choose a reason for hiding this comment

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

This isn't needed, you can just use wholesym::samply_symbols in the code - it won't make a difference but I think it's good practice to use the re-export in order to avoid mixing versions.

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