Closed
Bug 637316
Opened 14 years ago
Closed 14 years ago
Minidumps don't contain all mappings for elfhacked libraries
Categories
(Toolkit :: Crash Reporting, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: glandium, Assigned: glandium)
References
Details
(Whiteboard: fixed-in-bs)
Attachments
(3 files, 1 obsolete file)
2.08 KB,
patch
|
mwu
:
review+
|
Details | Diff | Splinter Review |
2.36 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
4.53 KB,
patch
|
Details | Diff | Splinter Review |
With elfhack, there are two PT_LOAD segments to map the executable code instead of one on standard binaries. More precisely, there is one for the headers, dynamic symbols, etc. up to relocations, and one for the code. Minidumps only contain information for the first one (the least relevant), and this leads to crash dump bustages (see bug 637243).
Assignee | ||
Comment 1•14 years ago
|
||
This is where the problem is: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/minidump_writer.cc#828
Assignee | ||
Comment 2•14 years ago
|
||
(In reply to comment #1) > http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/minidump_writer.cc#828 Removing this line results in the proper information being written in the minidump, but stackwalk can't make sense off it.
Assignee | ||
Comment 3•14 years ago
|
||
While working on programs for bug 637680, I figured what needed to be done for this bug. We actually don't need to have breakpad supporting multiple mappings for a module. Reality is that even when there are several PT_LOADs (which is the common case, since there are usually 2), the dynamic linker allocates a single memory range and then maps each part corresponding to the PT_LOAD data inside that memory range. This is why there are private mappings with no permissions between each mapping on the same file for loaded libraries. So what we really need to do is to reconstruct this memory range. This is actually very trivial on Android (pushed something to try that I hope will work), and shouldn't be very hard in minidump_writer for linux. Note that at the moment, Android minidumps do include several mappings for each module, and that doesn't work properly, because the base address for the second mapping on a same file is wrong, which is why it so blatantly fail with elfhack on android, but differently than on linux. It hadn't made any problem so far because the second mapping is data, and we don't have functions there.
Assignee: nobody → mh+mozilla
Assignee | ||
Comment 4•14 years ago
|
||
See comment 3.
Attachment #516242 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 5•14 years ago
|
||
Tested on Linux. I'm going to test if it changes anything on Android (it really shouldn't)
Attachment #516278 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 6•14 years ago
|
||
> Tested on Linux. I'm going to test if it changes anything on Android (it really
> shouldn't)
... and it doesn't break anything on Android.
Comment 7•14 years ago
|
||
Comment on attachment 516242 [details] [diff] [review] part 1 - Report whole memory mapping to crash reporter for libraries on Android I am punting this to mwu.
Attachment #516242 -
Flags: review?(ted.mielczarek) → review?(mwu)
Comment 8•14 years ago
|
||
Comment on attachment 516242 [details] [diff] [review] part 1 - Report whole memory mapping to crash reporter for libraries on Android >+ report_mapping(si->name, si->base, (si->size + PAGE_MASK) & (~PAGE_MASK), 0); >+ si->size is already rounded up to the nearest page. (calculated in get_lib_extents) Were you able to verify that you could crash and get a stack? The previous mapping reporting gave direct mappings between addresses and offsets within the library file. With a single mapping, I'm not sure this is necessarily the case except for PT_LOAD sections where offset == vaddr. I'm guessing that the crash reporter stuff doesn't actually care about the offset within the library file so this is all fine.
Attachment #516242 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 9•14 years ago
|
||
(In reply to comment #8) > Were you able to verify that you could crash and get a stack? The previous > mapping reporting gave direct mappings between addresses and offsets within the > library file. With a single mapping, I'm not sure this is necessarily the case > except for PT_LOAD sections where offset == vaddr. I'm guessing that the crash > reporter stuff doesn't actually care about the offset within the library file > so this is all fine. I did test. Note the offset is actually very much irrelevant as it was reporting the file offset, and not the vaddr offset. Breakpad doesn't use that anyways. The good thing with having only one mapping for the whole module is that vaddrs *are* right for all the PT_LOADed segments, which was not the case before for all but the first segment.
Comment 10•14 years ago
|
||
(In reply to comment #9) > I did test. Note the offset is actually very much irrelevant as it was > reporting the file offset, and not the vaddr offset. Breakpad doesn't use that > anyways. The good thing with having only one mapping for the whole module is > that vaddrs *are* right for all the PT_LOADed segments, which was not the case > before for all but the first segment. Mmm ok, sounds about right. Guess we can also kill the offset arg in report_mapping.
Comment 11•14 years ago
|
||
Comment on attachment 516278 [details] [diff] [review] part 2 - Merge adjacent mappings with the same name into one module in crash reporter This needs to wind up upstream, but I can land it there for you, assuming it applies cleanly to Breakpad SVN. >diff --git a/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc b/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc >--- a/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc >+++ b/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc >@@ -319,35 +319,46 @@ LinuxDumper::EnumerateMappings(wasteful_ > uintptr_t start_addr, end_addr, offset; > > const char* i1 = my_read_hex_ptr(&start_addr, line); > if (*i1 == '-') { > const char* i2 = my_read_hex_ptr(&end_addr, i1 + 1); > if (*i2 == ' ') { > const char* i3 = my_read_hex_ptr(&offset, i2 + 6 /* skip ' rwxp ' */); > if (*i3 == ' ') { >+ const char* name = NULL; >+ // Only copy name if the name is a valid path name, or if >+ // we've found the VDSO image Can you fix this comment while you're here? Apparently Google style is to not use first-person pronouns, but there's a bit of inconsistency. You might say "or if this is the VDSO image" >+ // Merge adjacent mappings with the same name into one module, >+ // assuming they're a single library mapped by the dynamic linker >+ if (name && result->size()) { >+ MappingInfo* module = (*result)[result->size() - 1]; >+ if ((start_addr == module->start_addr + module->size) && >+ (my_strlen(name) == my_strlen(module->name)) && >+ (memcmp(name, module->name, my_strlen(name)) == 0)) { memcmp is usually an intrinsic, right? This file otherwise tries to avoid using libc or syscalls directly, to avoid entering the dynamic linker. Looks fine otherwise.
Attachment #516278 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 12•14 years ago
|
||
(In reply to comment #11) > Comment on attachment 516278 [details] [diff] [review] > part 2 - Merge adjacent mappings with the same name into one module in crash > reporter > > This needs to wind up upstream, but I can land it there for you, assuming it > applies cleanly to Breakpad SVN. Interestingly, it doesn't. I'll check the differences. > memcmp is usually an intrinsic, right? This file otherwise tries to avoid using > libc or syscalls directly, to avoid entering the dynamic linker. > > Looks fine otherwise. According to gcc documentation, it is not an intrisic, but it is a builtin. OTOH, there is my_strncmp().
Assignee | ||
Comment 13•14 years ago
|
||
Carrying over r+. Same patch, that applies on breakpad trunk (only difference was in the comment you mentioned, which you already fixed upstream), using my_strncmp instead of memcmp.
Attachment #516278 -
Attachment is obsolete: true
Attachment #519129 -
Flags: review+
Assignee | ||
Comment 14•14 years ago
|
||
http://hg.mozilla.org/projects/build-system/rev/d5bb42e60c93 http://hg.mozilla.org/projects/build-system/rev/0615993bf269 And a forgotten piece: http://hg.mozilla.org/projects/build-system/rev/e7ba53628e3e
Whiteboard: fixed-in-bs
Assignee | ||
Comment 15•14 years ago
|
||
As landed.
Assignee | ||
Updated•14 years ago
|
Attachment #519136 -
Attachment description: part 2 - Merge adjacent mappings with the same name into one module in crash reporter → part 2 - Merge adjacent mappings with the same name into one module in crash reporter, as landed
Comment 16•14 years ago
|
||
Landed upstream after I wrote a unittest: http://code.google.com/p/google-breakpad/source/detail?r=781
http://hg.mozilla.org/mozilla-central/rev/d5bb42e60c93 http://hg.mozilla.org/mozilla-central/rev/0615993bf269 http://hg.mozilla.org/mozilla-central/rev/e7ba53628e3e
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•