Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161713002:
Recover memory mappings before writing dump on ChromeOS (Closed)
|
Created:
4 years, 5 months ago by Ting-Yuan (Leo) Huang Modified:
4 years, 5 months ago CC:
google-breakpad-dev_googlegroups.com, Simon Que Base URL:
https://chromium.googlesource.com/breakpad/breakpad.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionRecover memory mappings before writing dump on ChromeOS
On Linux, breakpad relies on /proc/[pid]/maps to associate symbols from
addresses. ChromeOS' hugepage implementation replaces some segments
with anonymous private pages, which is a restriction of current
implementation in Linux kernel at the time of writing. Thus, breakpad
can no longer symbolize addresses from those text segments replaced by
hugepages.
This patch tries to recover the mappings. Because hugepages are always
inserted in between some .text sections, it tries to infer the names and
offsets of the segments, by looking at segments immediately precede and
succeed them.
For example, a text segment before hugepage optimization
02001000-03002000 r-xp /opt/google/chrome/chrome
can be broken into
02001000-02200000 r-xp /opt/google/chrome/chrome
02200000-03000000 r-xp
03000000-03002000 r-xp /opt/google/chrome/chrome
BUG=crbug.com/628040
R=mark@chromium.org
Committed: https://chromium.googlesource.com/breakpad/breakpad/+/41b91d064ecda6cbcc1eb9b8fe1a1f999b6188b7
Patch Set 1 #
Total comments: 15
Messages
Total messages: 13 (4 generated)
laszio@chromium.org changed reviewers: + llozano@chromium.org, mark@chromium.org
LGTM https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... File src/client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:518: // Merge adjacent mappings with the same name into one module, Would it have been easier to work the change in here?
wez@chromium.org changed reviewers: + wez@chromium.org
Drive-by comments; feel free to ignore as necessary. ;) https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... File src/client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:113: // crbug.com/628040 ChromeOS' use of hugepages confuses crash symbolization Do/will we have a bug filed to remove this hack if/when Linux supports remapping executable pages to hugepages, or better still does it dynamically itself? https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:124: // Case 1: curr, next What do these "cases" mean? Do we sometimes see Chrome with two segments and sometimes with three, in minidumps? The example listed above only shows the three-segment case. When do we see two? https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:128: // curr.size is a multiple of 2M. Do we need to be this specific about the sizes? How likely is it that we have contiguous mapped & anonymous segments? Could we avoid making assumptions about hugepage constants if we took the layout of the executable into account (e.g. if we see a contiguous anonymous+mapped pair and the offset into the source file matches, is that sufficient?) https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:132: void TryRecoverMappings(MappingInfo *curr, MappingInfo *next) { Looks like this function actually checks whether |curr| can be treated as part of |next| - should it be called TryTwoWayMerge() for example? https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:133: // Merged segments are marked with size = 0. It's not clear from the context that it's possible to reach here having already merged some segments. I'd assumed that we generally expect to merge at-most three segments into a single one, given the way that hugepages work right now? https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:144: curr->size == next->offset) { Strongly suggest turning this into a set of early-exit conditions, and commenting on what each check is intended to achieve, rather than having a big block comment and then a big block if. https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:146: // matched nit: This comment doesn't add any information :P https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:149: // (curr, next) nit: Nor does this one ;) https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:210: // This function tries to merge segemnts into the first entry, typo: segments https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:212: // See LinuxDumper::EnumerateMappings(). nit: EnumerateMappings() doesn't have any documentating comment to define its behaviour, though, so it's not clear what you're asking the reader to see there? https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:217: size_t r = mappings.size(); nit: Please use descriptive variable names, for readability. :) https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:224: l = m + 1; nit: I'd recommend using {} around the two cases here, for readability. https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:227: // Try to merge segments into the first. It's not clear to me what this is actually trying to do; I see the comment says "merge segments into the first entry.." but _why_ are we expecting to be able to merge mappings[0] into some arbitrary later segment, and why do we need to do that outside the scope of the loops below? Is there something special about the first entry? Does EnumerateMappings() guarantee that the first entry is the chrome exe, for example (though I see a comment in that function that implies that is not generally the case ;)?
https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... File src/client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... src/client/linux/minidump_writer/linux_dumper.cc:241: TryRecoverMappings(mappings[i], mappings[i + 1], mappings[i + 2]); If we already ran through trying to merge (anon, mapped) into single mapped entries then won't we miss three-entry (mapped, anon, mapped) cases now)?
On 2016/07/18 22:03:29, Mark Mentovai wrote: > LGTM > Thanks! > https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... > File src/client/linux/minidump_writer/linux_dumper.cc (right): > > https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... > src/client/linux/minidump_writer/linux_dumper.cc:518: // Merge adjacent mappings > with the same name into one module, > Would it have been easier to work the change in here? I'm not sure. It seems to me that iterate through it when all the info are parsed is easier, because the segment containing entry point can be moved and we need to examine segments before and after it. I'll try to see if I can find a elegant way to do this on-the-fly, with wez's comments addressed later. Shall we land this patch for the p0 bug first?
On 2016/07/18 22:48:29, Ting-Yuan (Leo) Huang wrote: > On 2016/07/18 22:03:29, Mark Mentovai wrote: > > LGTM > > > > Thanks! > > > > https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... > > File src/client/linux/minidump_writer/linux_dumper.cc (right): > > > > > https://codereview.chromium.org/2161713002/diff/1/src/client/linux/minidump_w... > > src/client/linux/minidump_writer/linux_dumper.cc:518: // Merge adjacent > mappings > > with the same name into one module, > > Would it have been easier to work the change in here? > > I'm not sure. It seems to me that iterate through it when all the info are > parsed is easier, because the segment containing entry point can be moved and we > need to examine segments before and after it. I'll try to see if I can find a > elegant way to do this on-the-fly, with wez's comments addressed later. Shall we > land this patch for the p0 bug first? I think we should land this as it is. We already have an LKGM.. The bug is very high priority.
To be sure, these are the test steps I did: 1. login with guest/a normal account. 2. visit chrome://crash 3. scp DUT:/home/chronos/user/crash/FILENAME.dmp . 4. minidump_dump FILENAME.dmp 5. verify that there is only one MDRawModule (see [1] for example) containing /opt/google/chrome/chrome 6. verify that Stream MD_LINUX_MAPS has multiple r-xp /opt/google/chrome/chrome Are there more tests I can/should do? [1] MinidumpModuleList module_count = 59 module[0] MDRawModule base_of_image = 0x7f659d39e000 size_of_image = 0x87a4000 checksum = 0x0 time_date_stamp = 0x0 1970-01-01 00:00:00 module_name_rva = 0x12dc8 version_info.signature = 0x0 version_info.struct_version = 0x0 version_info.file_version = 0x0:0x0 version_info.product_version = 0x0:0x0 version_info.file_flags_mask = 0x0 version_info.file_flags = 0x0 version_info.file_os = 0x0 version_info.file_type = 0x0 version_info.file_subtype = 0x0 version_info.file_date = 0x0:0x0 cv_record.data_size = 24 cv_record.rva = 0x12db0 misc_record.data_size = 0 misc_record.rva = 0x0 (code_file) = "/opt/google/chrome/chrome" (code_identifier) = "3c89e0e07311cf596164773821c72ae9c0353acc" (cv_record).cv_signature = 0x4270454c (cv_record).build_id = 3c89e0e07311cf596164773821c72ae9c0353acc (misc_record) = (null) (debug_file) = "/opt/google/chrome/chrome" (debug_identifier) = "E0E0893C117359CF6164773821C72AE90" (version) = ""
Yes, I’m going to land this in the Breakpad repository now to unblock things. You’ll need to do a DEPS roll for Breakpad in Chrome (or a buildspec roll on your branch) to pick this up.
Description was changed from ========== Recover memory mappings before writing dump on ChromeOS On Linux, breakpad relies on /proc/[pid]/maps to associate symbols from addresses. ChromeOS' hugepage implementation replaces some segments with anonymous private pages, which is a restriction of current implementation in Linux kernel at the time of writing. Thus, breakpad can no longer symbolize addresses from those text segments replaced by hugepages. This patch tries to recover the mappings. Because hugepages are always inserted in between some .text sections, it tries to infer the names and offsets of the segments, by looking at segments immediately precede and succeed them. For example, a text segment before hugepage optimization 02001000-03002000 r-xp /opt/google/chrome/chrome can be broken into 02001000-02200000 r-xp /opt/google/chrome/chrome 02200000-03000000 r-xp 03000000-03002000 r-xp /opt/google/chrome/chrome BUG=crbug.com/628040 ========== to ========== Recover memory mappings before writing dump on ChromeOS On Linux, breakpad relies on /proc/[pid]/maps to associate symbols from addresses. ChromeOS' hugepage implementation replaces some segments with anonymous private pages, which is a restriction of current implementation in Linux kernel at the time of writing. Thus, breakpad can no longer symbolize addresses from those text segments replaced by hugepages. This patch tries to recover the mappings. Because hugepages are always inserted in between some .text sections, it tries to infer the names and offsets of the segments, by looking at segments immediately precede and succeed them. For example, a text segment before hugepage optimization 02001000-03002000 r-xp /opt/google/chrome/chrome can be broken into 02001000-02200000 r-xp /opt/google/chrome/chrome 02200000-03000000 r-xp 03000000-03002000 r-xp /opt/google/chrome/chrome BUG=crbug.com/628040 R=mark@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/41b91d064ecda6cbcc1eb9b... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 41b91d064ecda6cbcc1eb9b8fe1a1f999b6188b7 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Recover memory mappings before writing dump on ChromeOS On Linux, breakpad relies on /proc/[pid]/maps to associate symbols from addresses. ChromeOS' hugepage implementation replaces some segments with anonymous private pages, which is a restriction of current implementation in Linux kernel at the time of writing. Thus, breakpad can no longer symbolize addresses from those text segments replaced by hugepages. This patch tries to recover the mappings. Because hugepages are always inserted in between some .text sections, it tries to infer the names and offsets of the segments, by looking at segments immediately precede and succeed them. For example, a text segment before hugepage optimization 02001000-03002000 r-xp /opt/google/chrome/chrome can be broken into 02001000-02200000 r-xp /opt/google/chrome/chrome 02200000-03000000 r-xp 03000000-03002000 r-xp /opt/google/chrome/chrome BUG=crbug.com/628040 R=mark@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/41b91d064ecda6cbcc1eb9b... ========== to ========== Recover memory mappings before writing dump on ChromeOS On Linux, breakpad relies on /proc/[pid]/maps to associate symbols from addresses. ChromeOS' hugepage implementation replaces some segments with anonymous private pages, which is a restriction of current implementation in Linux kernel at the time of writing. Thus, breakpad can no longer symbolize addresses from those text segments replaced by hugepages. This patch tries to recover the mappings. Because hugepages are always inserted in between some .text sections, it tries to infer the names and offsets of the segments, by looking at segments immediately precede and succeed them. For example, a text segment before hugepage optimization 02001000-03002000 r-xp /opt/google/chrome/chrome can be broken into 02001000-02200000 r-xp /opt/google/chrome/chrome 02200000-03000000 r-xp 03000000-03002000 r-xp /opt/google/chrome/chrome BUG=crbug.com/628040 R=mark@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/41b91d064ecda6cbcc1eb9b... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
