Conversation
Expose parts of set_process_dispatcher in the capi to enable users to provide alternative ELF file paths for symbolization (e.g., fetched via debuginfod). Signed-off-by: Arne Jansen <arne@die-jansens.de>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1535 +/- ##
==========================================
+ Coverage 95.48% 95.52% +0.03%
==========================================
Files 56 56
Lines 9555 9555
==========================================
+ Hits 9124 9127 +3
+ Misses 431 428 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// the underlying language does not mangle symbols (such as C). | ||
| pub demangle: bool, | ||
| /// Explicit pad to avoid implicit gaps. | ||
| pub _pad_a: [u8; 4], |
There was a problem hiding this comment.
Can we rename this to _reserved1 or so?
| /// languages are Rust and C++ and the flag will have no effect if | ||
| /// the underlying language does not mangle symbols (such as C). | ||
| pub demangle: bool, | ||
| /// Explicit pad to avoid implicit gaps. |
There was a problem hiding this comment.
Can you please make it clear that this should be initialized to zero like other unused fields?
| /// Unused member available for future expansion. Must be initialized | ||
| /// to zero. | ||
| pub reserved: [u8; 20], | ||
| pub reserved: [u8; 24], |
There was a problem hiding this comment.
I think the size of the struct should stay constant. So we should decrease instead of increase. It's a bit annoying that this change alone brings us to the limit, basically. Perhaps we'd should just embed a single pointer to a struct containing the two pointers in question...? Given that it's a niche case and can be stack allocated, it's probably okay, would be my take.
|
|
||
| let builder = if let Some(cb) = process_dispatch_cb { | ||
| // Cast the context pointer to usize so the closure is Send. | ||
| // The caller is responsible for thread safety of the context. |
There was a problem hiding this comment.
Okay, but that should be mentioned in the corresponding part of the documentation then, I'd say.
| // NUL-terminated, `malloc`'d C string. | ||
| let path_cstr = unsafe { CStr::from_ptr(result) }; | ||
| let path = Path::new(OsStr::from_bytes(path_cstr.to_bytes())); | ||
| let resolver = ElfResolver::open(path); |
There was a problem hiding this comment.
This constructor doesn't honor any custom debug directories that may have been configured. I feel like this can be confusing to users relying on that.
There was a problem hiding this comment.
I see. I also found that the caching behavior is different, it cannot de-duplicate between processes anymore if I read this correctly. so how do we solve this? I see
- Just document the behavior.
- Add another constructor to
ElfResolverthat also passesdebug_dirsand leave caching as-is. - Somehow pass the path back to the symbolizer and let it construct the resolver the same way it would without the dispatch callback. This would handle both issues.
I find 3 to be the most appealing, but also the most intrusive. What do you think?
There was a problem hiding this comment.
Yeah, I think the caching story is unfortunate but non-trivial to solve. Given that it's not really a correctness issue, I'd say we leave it as-is. For debug_dirs, I think an additional constructor for ElfResolver is fine.
2 participants
Expose parts of set_process_dispatcher in the capi to enable users to provide alternative ELF file paths for symbolization (e.g., fetched via debuginfod).
As discussed in #1443.
Please let me know if this goes in the direction you had in mind.