Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(634)

Issue 5754002: Introduce microdump writer class. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 5 months ago by primiano
Modified:
9 years, 5 months ago
Reviewers:
mmandlis, Mark Mentovai
CC:
google-breakpad-dev_googlegroups.com, simonb
Base URL:
http://google-breakpad.googlecode.com/svn/trunk
Visibility:
Public.

Description

Introduce microdump writer class.

Microdumps are a very lightweight variant of minidumps. They are meant
to dump a minimal crash report on the system log (logcat on Android),
containing only the state of the crashing thread.
This is to deal with cases where the user has opted out from crash
uploading but we still want to generate meaningful information on the
device to pull a stacktrace for development purposes.
Conversely to conventional stack traces (e.g. the one generated by
Android's debuggerd or Chromium's base::stacktrace) microdumps do NOT
require unwind tables to be present in the target binary. This allows
to save precious binary size (~1.5 MB for Chrome on Arm, ~10 MB on
arm64).
More information and design doc on crbug.com/410294

BUG=chromium:410294

Patch Set 1 #

Total comments: 16

Patch Set 2 : Address review + update makefiles #

Total comments: 20

Patch Set 3 : Address new Maria's comments. #

Total comments: 6

Patch Set 4 : Rework minidump_descriptor interface #

Patch Set 5 : Introduce internal mode for minidump_descriptor #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M Makefile.am View 1 2 chunks +2 lines, -0 lines 0 comments Download
M Makefile.in View 1 10 chunks +19 lines, -2 lines 0 comments Download
M src/client/linux/dump_writer_common/mapping_info.h View 1 2 2 chunks +12 lines, -0 lines 0 comments Download
M src/client/linux/handler/exception_handler.cc View 1 2 3 4 chunks +11 lines, -2 lines 0 comments Download
M src/client/linux/handler/minidump_descriptor.cc View 1 2 3 4 4 chunks +8 lines, -3 lines 0 comments Download
M src/client/linux/handler/minidump_descriptor.h View 1 2 3 4 4 chunks +37 lines, -8 lines 0 comments Download
A src/client/linux/microdump_writer/microdump_writer.cc View 1 2 1 chunk +364 lines, -0 lines 0 comments Download
A + src/client/linux/microdump_writer/microdump_writer.h View 1 2 1 chunk +21 lines, -12 lines 0 comments Download
M src/client/linux/minidump_writer/minidump_writer.h View 1 1 chunk +0 lines, -8 lines 0 comments Download

Messages

Total messages: 15
primiano
There we go... finally getting to the juicy content. Maria would you mind start taking ...
9 years, 5 months ago #1
mmandlis
I haven't looked in details into the microdump_writer code yet, but I'm sending you my ...
9 years, 5 months ago #2
primiano
Thanks for the review. It should look in a better state now. https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/exception_handler.cc File src/client/linux/handler/exception_handler.cc ...
9 years, 5 months ago #3
mmandlis
looks good overall. couple more nits, but mostly questions to help me better internalize this ...
9 years, 5 months ago #4
primiano
Thanks. See replies inline. Mark: can I ask you a comment about minidump_descriptor.h ? Does ...
9 years, 5 months ago #5
Mark Mentovai
https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/minidump_descriptor.h File src/client/linux/handler/minidump_descriptor.h (right): https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/minidump_descriptor.h#newcode41 src/client/linux/handler/minidump_descriptor.h:41: // either a file descriptor, a path or a ...
9 years, 5 months ago #6
mmandlis
I'm happy with all the other changes, and +1 to Mark's comments.
9 years, 5 months ago #7
primiano
Many thanks Mark for the comments. Does it look better now? https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/minidump_descriptor.h File src/client/linux/handler/minidump_descriptor.h (right): ...
9 years, 5 months ago #8
Mark Mentovai
primiano wrote: > > A better option would be to make the operating mode much ...
9 years, 5 months ago #9
primiano
Oh, I completely misunderstood the suggestion. Makes sense. Done!
9 years, 5 months ago #10
Mark Mentovai
That looks like the way to go now! I only looked at minidump_descriptor.h.
9 years, 5 months ago #11
mmandlis
lgtm Btw, are you going to add some tests for the microdump_writer class? I'm ok ...
9 years, 5 months ago #12
primiano
> Btw, are you going to add some tests for the microdump_writer class? I'm ok ...
9 years, 5 months ago #13
mmandlis
Tests - are the best way to check whether a person can keep his promise))) ...
9 years, 5 months ago #14
primiano
9 years, 5 months ago #15
Message was sent while issue was closed.
Committed as r1398
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1004:630ec63f810e-tainted