Repository navigation
Conversation
|
@gabrielesvelto @Jake-Shadle You're going to want to review only ea7b747. The other two commits are from the other two open PRs. |
|
Gonna review this ASAP. Thank you @marti4d |
|
BTW we might want to add a "validation" step to the list of modules we've generated, to spot problematic cases like those we encountered in the wild. For example, if we find two modules with the same name but different debug IDs. |
fcd1ac4 to
350da9b
Compare
There was a problem hiding this comment.
Mostly stylistic/readability changes but I was also curious about the hard-coded page size in the code.
|
@marti4d do you want to double-check the Android failures or shall I merge? |
|
I am going to spend some time trying to look into them, but it's weird. I don't see the issue in my local emulator, or on real Android devices. So it makes me wonder if there's something going on with CI, or if the emulator is just running too slow on weak hardware? Either way, maybe I should try even more to figure it out before we merge. |
fee5874 to
87483da
Compare
| /// Address of callback function for module map/unmap | ||
| r_brk: usize, | ||
| /// Argument to r_brk on whether mapping is being added, subtracted, or is done | ||
| r_state: u8, |
There was a problem hiding this comment.
Looking at the bionic source code, this field is an i32, not a u8:
https://android.googlesource.com/platform/bionic/+/a27d2baa/linker/linker.h#49
There was a problem hiding this comment.
Oh my, it's an enum in glibc so... I guess it's different between Android and Linux? Since it only has three value I guess that GCC defaults this as a u8 when compiling glibc but I'd like to verify that first.
There was a problem hiding this comment.
I played a little bit with godbolt and both clang and GCC will lay out r_state as a 32-bit field on Linux too, so let's go for that.
There was a problem hiding this comment.
I managed to reproduce the hang locally, so I tried changing that field to a i32 and the hang disappeared. Now I'm chasing a new failure :)
|
Superseded by #195 |
| #[allow(non_camel_case_types)] | ||
| #[repr(C)] | ||
| #[derive(Debug)] | ||
| struct r_debug { |
There was a problem hiding this comment.
@marti4d I know it's been a while since you wrote this, but is there a reason you didn't use the similar definitions in src/linux/dso_debug.rs? I just stumbled upon them by accident.
There was a problem hiding this comment.
Honestly? I was very new to minidump-writer at that point and probably didn't realize it was even there. I probably did a quick grep for r_debug, didn't see any hits, and wrote my own.
But yep -- Now that I know it's there, we should probably just prefer to use that instead.
Fixes #142