Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1535 +/- ##
==========================================
+ Coverage 95.52% 95.65% +0.13%
==========================================
Files 56 56
Lines 9534 9562 +28
==========================================
+ Hits 9107 9147 +40
+ Misses 427 415 -12 ☔ 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.
There was a problem hiding this comment.
I eliminated the cast, as it doesn't really gain anything in the current thread model, if I understand it correctly.
| // 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 (a solution is on the roadmap for 0.3). For debug_dirs, I think an additional constructor for ElfResolver is fine.
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>
Signed-off-by: Arne Jansen <arne@die-jansens.de>
|
Thanks for the review. I hope I addressed everything. Pushed the changes as a fixup commit. |
d-e-s-o
left a comment
There was a problem hiding this comment.
Overall this looks good to me now, thanks. Can you take a look at two more comments below?
|
|
||
| /// Symbolize `addr` in the current process using the given dispatch | ||
| /// callback. | ||
| unsafe fn symbolize_with_dispatch( |
There was a problem hiding this comment.
I fail to see why this function needs to be unsafe?
| /// Create an `ElfResolver` that loads data from the provided file, | ||
| /// using the given directories to search for split debug | ||
| /// information. | ||
| pub fn open_with_debug_dirs<P, D, DP>(path: P, debug_dirs: D) -> Result<Self> |
There was a problem hiding this comment.
I think this should be only available if the dwarf feature is enabled, same as https://docs.rs/blazesym/latest/blazesym/symbolize/struct.Builder.html#method.set_debug_dirs.
d-e-s-o
left a comment
There was a problem hiding this comment.
Two more comments, actually.
| } | ||
| // SAFETY: The caller ensures that the pointer is valid and the count | ||
| // matches. | ||
| let slice = unsafe { slice_from_user_array(debug_dirs, _debug_dirs_len) }; |
There was a problem hiding this comment.
Please also rename _debug_dirs_len to debug_dirs_len given that it's now used.
| #[derive(Debug)] | ||
| pub struct blaze_symbolizer_dispatch { | ||
| /// The dispatch callback function. Must not be `NULL`. | ||
| pub dispatch_cb: unsafe extern "C" fn( |
There was a problem hiding this comment.
I think this should still be wrapped in an Option to be NULL-able, no?
There was a problem hiding this comment.
I'm not sure I get that. If the callback is not needed, process_dispatch is allowed to be NULL. Is that something about FFI semantics?
There was a problem hiding this comment.
Yes, a fn cannot safely be NULL. It's like a reference; can't create it from a NULL ptr. At least that is my understanding.
There was a problem hiding this comment.
There was a problem hiding this comment.
I get that if we want to allow the callback to be NULL, or even be able to check if it is NULL, we are required to add the Option. But as we don't allow a NULL cb here, is there still any advantage? As it doesn't have any downsides, I'll just add Option. Just curious.
There was a problem hiding this comment.
Ah, I see now what you are saying. You are correct. You were referring to the "outer pointer" being NULL. Then we can keep things as-is on this front. Sorry for the confusion.
Signed-off-by: Arne Jansen <arne@die-jansens.de>
|
Pushed fixup 2 |
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.