Support processing microdump for mips architecture

Support processing microdump for mips architecture

Based on changes for ARM, ARM64 and X86, the support for
MIPS and MIPS64 is added in microdump.

TEST=microdump_stackwalk  ~/microdump-mips32.dmp symbols/
BUG=microdump_stackwalk failing for mips architectures

[mveljko, 2016-02-24 13:09:37]

veljko.mihailovic@imgtec.com changed reviewers:
+ primiano@chromium.org, vapier@chromium.org

[mveljko, 2016-02-24 13:09:38]

Please take a look

[vapier, 2016-02-24 22:35:44]

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc
File src/processor/microdump.cc (right):

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:137: void
MicrodumpContext::SetContextMIPS(MDRawContextMIPS* mips, bool arch64) {
i haven't looked closely, but why do we need these SetContext helpers at all ? 
we only have them for arm/arm64/x86 and not other arches.  do we only support
microdumps for those three arches currently ?

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:324: strcmp(arch.c_str(), kMips64Architecture) == 0)
{
indentation looks off here

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:327: " bytes instead of " << sizeof(MDRawContextMIPS)
<< std::endl;
indentation looks off here

[Primiano Tucci (use gerrit), 2016-02-25 01:08:07]

Fun... I didn't remember writing the code for producing  mips microdumps, but
according to git log I did :)

The patch looks good to me, I'd just ask to address few of minor comments before
landing this. Many thanks for spending the time for the tests, they are
definitely appreciated.

Just one thing: I clearly didn't test all the microdump generation path: did you
test it? did you get that microdump in the test from an actual breakpad crash
(looks so)?

https://codereview.chromium.org/1731923002/diff/1/src/google_breakpad/process...
File src/google_breakpad/processor/microdump.h (right):

https://codereview.chromium.org/1731923002/diff/1/src/google_breakpad/process...
src/google_breakpad/processor/microdump.h:70: virtual void
SetContextMIPS(MDRawContextMIPS* mips, bool arch64);
Can you keep this consistent and have SetContextMIPS and SetContextMIPS64.
Boolean flags are typically not super legible.

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc
File src/processor/microdump.cc (right):

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:68: 
nit: spurious extra line?

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump_pro...
File src/processor/microdump_processor_unittest.cc (right):

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump_pro...
src/processor/microdump_processor_unittest.cc:223: " 10 15:20:59 PDT 2015",
state.system_info()->os_version);
Looks like this string should fit on one line, why splitting this?
I mean, looks to me this should be:

 ASSERT_EQ("3.0.8-g893bf16 #7 SMP PREEMPT Fri Jul 10 15:20:59 PDT 2015",
                        state.system_info()->os_version);

[mveljko, 2016-02-25 09:29:51]

I addressed all the comments from the previous patch set.
I don't know why git saw that those .dmp and .sym files were changed, when they
were not.

Also to answer Primiano's question:

Yes, I tested the whole path. It is an actual breakpad crash from my mips and
mips64 devices. I used the crash_example provided with breakpad code for android
and modified it so that instead of a minidump, I get the microdump.

And one note/question from me:
I noticed that it is possible to create microdump for android and linux devices.
I was successfull in creating for both os.

But the problem is that microdump_stackwalker in it's current implementation,
can only go through microdumps created on android devices because of logcat
prefix.
Here is the specific line:
https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/src/proce...

Is this done on purpose? Because I know that microdump is initially created for
android devices when user turns off crash reporter.

https://codereview.chromium.org/1731923002/diff/1/src/google_breakpad/process...
File src/google_breakpad/processor/microdump.h (right):

https://codereview.chromium.org/1731923002/diff/1/src/google_breakpad/process...
src/google_breakpad/processor/microdump.h:70: virtual void
SetContextMIPS(MDRawContextMIPS* mips, bool arch64);
On 2016/02/25 01:08:06, Primiano (throttled til Mar 4) wrote:
> Can you keep this consistent and have SetContextMIPS and SetContextMIPS64.
> Boolean flags are typically not super legible.

Done. I separated in two methods.

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc
File src/processor/microdump.cc (right):

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:68: 
On 2016/02/25 01:08:07, Primiano (throttled til Mar 4) wrote:
> nit: spurious extra line?

Done. Removed it.

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:137: void
MicrodumpContext::SetContextMIPS(MDRawContextMIPS* mips, bool arch64) {
On 2016/02/24 22:35:43, vapier wrote:
> i haven't looked closely, but why do we need these SetContext helpers at all ?

> we only have them for arm/arm64/x86 and not other arches.  do we only support
> microdumps for those three arches currently ?
I don't know really why, maybe so that the code would look nicer, or some other
reason. I found the state like this, and just followed the way it is functioning
so that the implementation for MIPS would be the same.

As for microdumps, yes, now only those 3 architectures are supported.

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:324: strcmp(arch.c_str(), kMips64Architecture) == 0)
{
On 2016/02/24 22:35:43, vapier wrote:
> indentation looks off here

Fixed this when I separated the functions.

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:327: " bytes instead of " << sizeof(MDRawContextMIPS)
<< std::endl;
On 2016/02/24 22:35:43, vapier wrote:
> indentation looks off here

I checked the if branches for arm/arm64/x86 and the indentation there is the
same, I just followed it, 4 spaces.

If you like I can change it for every if branch.

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump_pro...
File src/processor/microdump_processor_unittest.cc (right):

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump_pro...
src/processor/microdump_processor_unittest.cc:223: " 10 15:20:59 PDT 2015",
state.system_info()->os_version);
On 2016/02/25 01:08:06, Primiano (throttled til Mar 4) wrote:
> Looks like this string should fit on one line, why splitting this?
> I mean, looks to me this should be:
> 
>  ASSERT_EQ("3.0.8-g893bf16 #7 SMP PREEMPT Fri Jul 10 15:20:59 PDT 2015",
>                         state.system_info()->os_version);

Done. Fixed it for both tests.

[vapier, 2016-02-25 16:15:44]

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc
File src/processor/microdump.cc (right):

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:327: " bytes instead of " << sizeof(MDRawContextMIPS)
<< std::endl;
this looks like the only file that wraps cout/cerr commands.  i'd expect it to
look like:

  std::cerr << "Malformed CPU context. Got " << cpu_state_raw.size()
            << " bytes instead of " << sizeof(MDRawContextMIPS) << std::endl;

so the << would be aligned

[mveljko, 2016-02-25 16:26:15]

On 2016/02/25 16:15:44, vapier wrote:
> https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc
> File src/processor/microdump.cc (right):
> 
>
https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
> src/processor/microdump.cc:327: " bytes instead of " <<
sizeof(MDRawContextMIPS)
> << std::endl;
> this looks like the only file that wraps cout/cerr commands.  i'd expect it to
> look like:
> 
>   std::cerr << "Malformed CPU context. Got " << cpu_state_raw.size()
>             << " bytes instead of " << sizeof(MDRawContextMIPS) << std::endl;
> 
> so the << would be aligned

So then, should I fix in whole file, not just MIPS part?

[mveljko, 2016-02-25 16:35:25]

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc
File src/processor/microdump.cc (right):

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:327: " bytes instead of " << sizeof(MDRawContextMIPS)
<< std::endl;
On 2016/02/25 16:15:44, vapier wrote:
> this looks like the only file that wraps cout/cerr commands.  i'd expect it to
> look like:
> 
>   std::cerr << "Malformed CPU context. Got " << cpu_state_raw.size()
>             << " bytes instead of " << sizeof(MDRawContextMIPS) << std::endl;
> 
> so the << would be aligned

So should I fix this in the whole file or just MIPS part?

[Primiano Tucci (use gerrit), 2016-02-25 17:00:15]

LGTM % vapier nits. clang-format might help :)

>And one note/question from me:
> I noticed that it is possible to create microdump for android and linux
devices.
> I was successfull in creating for both os.
> But the problem is that microdump_stackwalker in it's current implementation,
> can only go through microdumps created on android devices because of logcat
> prefix.
>Is this done on purpose? Because I know that microdump is initially created for
>android devices when user turns off crash reporter.
So the reason is that I never had a use case to use microdumps on Linux, so
never bothered writing the code for parsing that.
Do you have a strong use case for using that on Linux. I am a bit recluctant to
change the processor parsing code unless we have a strong use case (technically
somebody else owns it). if we screw up something there might lose lots of crash
reports.

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc
File src/processor/microdump.cc (right):

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:327: " bytes instead of " << sizeof(MDRawContextMIPS)
<< std::endl;
On 2016/02/25 16:35:25, mveljko wrote:
> On 2016/02/25 16:15:44, vapier wrote:
> > this looks like the only file that wraps cout/cerr commands.  i'd expect it
to
> > look like:
> > 
> >   std::cerr << "Malformed CPU context. Got " << cpu_state_raw.size()
> >             << " bytes instead of " << sizeof(MDRawContextMIPS) <<
std::endl;
> > 
> > so the << would be aligned
> 
> So should I fix this in the whole file or just MIPS part?

Fix also the others.
I think it's easier if you run clang-format on this file, will fix it for you.

[mveljko, 2016-02-26 10:43:55]

> So the reason is that I never had a use case to use microdumps on Linux, so
> never bothered writing the code for parsing that.
> Do you have a strong use case for using that on Linux. I am a bit recluctant
to
> change the processor parsing code unless we have a strong use case
(technically
> somebody else owns it). if we screw up something there might lose lots of
crash
> reports.

No I don't have a specific Linux use case. I just noticed that microdumps can be
created on Linux, which was unexpected, but could not be processed by microdump
stackwalker.
Maybe microdump generation should be disabled on Linux since it currently can be
misleading.

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc
File src/processor/microdump.cc (right):

https://codereview.chromium.org/1731923002/diff/1/src/processor/microdump.cc#...
src/processor/microdump.cc:327: " bytes instead of " << sizeof(MDRawContextMIPS)
<< std::endl;
On 2016/02/25 17:00:15, Primiano (throttled til Mar 4) wrote:
> On 2016/02/25 16:35:25, mveljko wrote:
> > On 2016/02/25 16:15:44, vapier wrote:
> > > this looks like the only file that wraps cout/cerr commands.  i'd expect
it
> to
> > > look like:
> > > 
> > >   std::cerr << "Malformed CPU context. Got " << cpu_state_raw.size()
> > >             << " bytes instead of " << sizeof(MDRawContextMIPS) <<
> std::endl;
> > > 
> > > so the << would be aligned
> > 
> > So should I fix this in the whole file or just MIPS part?
> 
> Fix also the others.
> I think it's easier if you run clang-format on this file, will fix it for you.

I ran the file through clang-format and the result is in patch-set #3

There are a number of changes in the file, so in my opinion, this should be done
for the complete breakpad project and committed in a separate change.

[Primiano Tucci (use gerrit), 2016-02-26 17:32:23]

On 2016/02/26 10:43:55, mveljko wrote:
> Maybe microdump generation should be disabled on Linux since it currently can
be
> misleading.
I am not sure I follow. It is already disabled by default, unless you explicitly
call InitMicrodumpCrashHandlerIfNecessary().

> There are a number of changes in the file, so in my opinion, this should be
done
> for the complete breakpad project and committed in a separate change.
Oh, I see what's happening. If you run it from outside the chromium tree uses
the default clang-format rules.
Definitely we don't want to reformat all these changes. Please undo PS2 and just
fix the cout manually.
breakpad is currently in maintenance mode as it's going to be replaced by
crashpad. Not worth doing fancy stylistical changes at this point.
Just fix the indentation manually as vapier@ suggested then.

[vapier, 2016-02-26 19:05:45]

i'm not against us using a clang format file for the code base, but that should
be a sep CL

i'd like to get the rewrite sorted out before landing anymore changes, but
hopefully that won't take too much longer

[mveljko, 2016-02-29 09:10:06]

On 2016/02/26 19:05:45, vapier wrote:
> i'm not against us using a clang format file for the code base, but that
should
> be a sep CL
> 
> i'd like to get the rewrite sorted out before landing anymore changes, but
> hopefully that won't take too much longer

Fixed identation manualy, compare ps4 with ps2 to verify.

@Primiano
This is how I got microdmup on Linux console(changed crash example from
documentation).
  google_breakpad::MinidumpDescriptor
descriptor(google_breakpad::MinidumpDescriptor::kMicrodumpOnConsole);
  google_breakpad::ExceptionHandler eh(descriptor, NULL, DumpCallback,
                                       NULL, true, -1);
My thought was to disable this 3rd type of constructor for MinidumpDescriptor
unless it is built for
Android OS.
But probably no one will ever try to use this on Linux or any other OS.

[mveljko, 2016-02-29 09:17:08]

> But probably no one will ever try to use this on Linux or any other OS.

that is not Android.

[vapier, 2016-02-29 15:58:42]

what are the sizes of those sym files ?  i'm guessing they're not exactly small.
 can you trim unnecessary content from them ?  we're trying to avoid adding
large files to the repo anymore.

[vapier, 2016-02-29 17:40:40]

also, i suspect you'll need to rebase to latest master :)

[mveljko, 2016-03-01 13:18:21]

On 2016/02/29 15:58:42, vapier wrote:
> what are the sizes of those sym files ?  i'm guessing they're not exactly
small.
>  can you trim unnecessary content from them ?  we're trying to avoid adding
> large files to the repo anymore.

Code is rebased.

The sym files are ~700KB in size.
I would prefer keeping them as they are.

Please note that sym files for breakpad_unittests also located in
symbols/microdump are double that size.

[vapier, 2016-03-02 21:04:09]

existing large files are not a good reason to include more.  we want to scale
these back, not keep making it worse.

[mveljko, 2016-03-30 14:20:07]

On 2016/03/02 21:04:09, vapier wrote:
> existing large files are not a good reason to include more.  we want to scale
> these back, not keep making it worse.

Sorry for the delay, the files are reduced now.
PTAL

[vapier, 2016-03-30 22:33:13]

lgtm

[vapier, 2016-04-01 23:04:20]

pushed manually since `git cl land` breaks authorship info