Microdumps: refactor out common parts of minidump_writer.cc

Microdumps: refactor out common parts of minidump_writer.cc

This change is a pure refactoring of the common bits of minidump_writer.cc
that will be shared soon with the upcoming microdump_writer.cc.
In particular, this CL is extracting the following classes:
- ThreadInfo: handles the state of the threads in the crashing process.
- RawContextCPU: typedef for arch-specific CPU context structure.
- UContextReader: Fills out a dump RawContextCPU structure from the
  ucontext struct provided by the kernel (arch-dependent).
- SeccompUnwinder: cleans out the stack frames of the Seccomp sandbox
  on the supported architectures.
- MappingInfo: handles information about mappings

BUG=chromium:410294
R=mmandlis@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1388

[primiano]

Mark could you PTAL to see if makes sense to you at least at a higher level?
I think Maria can take care of line-by-line review.

Essentially I am extracting out the parts of the minidump_writer that I will
need also in the minidump writer.

Doubt: is it really worth creating a new dump_writer_common/ folder in your
opinion? Should I instead have everybody live happily together in
minidump_writer?
Essentially my two propsals here are:

client/linux/minidump_writer/linux_dumper.h
+ client/linux/minidump_writer/thread_info.h
+ client/linux/minidump_writer/mapping_info.h
+ (future CL) client/linux/minidump_writer/MICROdump_writer.cc

VS.

client/linux/minidump_writer/linux_dumper.h
+ client/linux/DUMP_WRITER_COMMON/thread_info.h
+ client/linux/DUMP_WRITER_COMMON/mapping_info.h
+ (future CL) client/linux/MICROdump_writer/MICROdump_writer.cc

This CL aims at the latter.

Thanks

[primiano]

Ah, this will require this matching upstream change:
https://codereview.chromium.org/602403002

(Internal committish of PS 1: 299ee712da)

[primiano]

Or eventually, I was thinking, I can just make this simple and cut it short, and
just have a class dump_writer_common.h/.cc (extended by both
mini/micro_dump_Writer) which has all the common methods.
Probably is just more readable, WDYT?

[mmandlis]

I'm all for simple and short, and I like the idea of dump_writer_common base
class.
I'm a bit hesitant about microdump_writer living in minidump_writer/ folder, but
i still think it is a reasonable enough compromise to live with. imho.

On 2014/09/26 17:00:21, primiano wrote:
> Or eventually, I was thinking, I can just make this simple and cut it short,
and
> just have a class dump_writer_common.h/.cc (extended by both
> mini/micro_dump_Writer) which has all the common methods.
> Probably is just more readable, WDYT?

[Mark Mentovai]

This seems all right to me.

I’m all for refactoring things into common locations, but I don’t know if a base
class is that awesome an idea if its only reason for existence is to provide a
handful of static methods. In that case, there doesn’t seem to be a fantastic
argument for inheritance.

[primiano]

On 2014/09/26 21:39:31, Mark Mentovai wrote:
> This seems all right to me.
> 
> I’m all for refactoring things into common locations, but I don’t know if a
base
> class is that awesome an idea if its only reason for existence is to provide a
> handful of static methods. In that case, there doesn’t seem to be a fantastic
> argument for inheritance.

Ok, I went for the clean way as suggested by Mark. It's a bit more effort but
looks definitely cleaner and more readable.

Maria, would you mind at this point taking a look in details to the CL?
There should be no surprises introduced. I essentially cut-pasted pieces of
minidump_writer into dump_writer_common/SomeClass and re-glued the dependencies
together. However I won't be surprised if I made some terrible cut/paste mistake
somewhere.
I tested locally building for the following platforms: desktop: x86_64, Android:
arm, arm64, mips, x86, x86_64.
At least... 7 compilers are still happy :)

[mmandlis]

lgtm, but please address the minor comments/questions i have before submitting.

i was really trying hard to find the "terrible copy&paste" errors, but it's
either that there are none, or i missed them.

and just to be clear, when you're saying that compilers are happy, you also mean
that all tests are still passing, right? 

btw, should you not be adding the new files to Makefile.am?

https://breakpad.appspot.com/4684002/diff/80001/src/client/linux/dump_writer_...
File src/client/linux/dump_writer_common/ucontext_reader.cc (right):

https://breakpad.appspot.com/4684002/diff/80001/src/client/linux/dump_writer_...
src/client/linux/dump_writer_common/ucontext_reader.cc:157: out->context_flags =
MD_CONTEXT_ARM_FULL;
same comment - this function appears under
#elif defined(__ARM_ARMEL__) in the original code.

https://breakpad.appspot.com/4684002/diff/80001/src/client/linux/minidump_wri...
File src/client/linux/minidump_writer/minidump_writer.cc (left):

https://breakpad.appspot.com/4684002/diff/80001/src/client/linux/minidump_wri...
src/client/linux/minidump_writer/minidump_writer.cc:328: #elif
defined(__ARMEL__)
what is the difference (if any) between __ARMEL__ and __ARM_EABI__?
In the new code, the equivalent of this CPUFillFromThreadInfo function appears
under #elif defined(__ARM_EABI__)
in thread_info.cc ln 192

https://breakpad.appspot.com/4684002/diff/80001/src/client/linux/minidump_wri...
src/client/linux/minidump_writer/minidump_writer.cc:477: #error "This code has
not been ported to your platform yet."
I don't think you used this 
#else
#error "..." 
in any of the resulting classes/files. Is this intentional?

[primiano]

Mark: organizational question: I don't seem to have the powers to commit this
change. How should we deal with this? Can I get the *investiture*? Should I ask
Maria to land it for me?

Maria:
I am amazed by the pedanticity of your review ^__^
Both the #if and the __ARM_EABI__ thing were intentional (see reply inline).
But, if you spotted them and you didn't spot anything else this is very
encouraging for me :)

+nealsid for the question about __ARM_EABI__ vs __ARM_ARMEL__

>and just to be clear, when you're saying that compilers are happy, you also
mean that all tests are still passing, right? 
I tested only on Linux desktop and they pass. I don't have 7 different
tablets/phones with 7 different platforms... yet (I'm getting close).


> btw, should you not be adding the new files to Makefile.am?
Ah absolutely right. I completely forgot about this and cared only about gyp.
Will do that now (before submitting).

https://breakpad.appspot.com/4684002/diff/80001/src/client/linux/dump_writer_...
File src/client/linux/dump_writer_common/ucontext_reader.cc (right):

https://breakpad.appspot.com/4684002/diff/80001/src/client/linux/dump_writer_...
src/client/linux/dump_writer_common/ucontext_reader.cc:157: out->context_flags =
MD_CONTEXT_ARM_FULL;
On 2014/09/30 01:42:58, mmandlis wrote:
> same comment - this function appears under
> #elif defined(__ARM_ARMEL__) in the original code.

Yeah that was intentional. The reason I did it is because that was the only
instance of __ARM_ARMEL__ in breakpad, which uses __ARM_EABI__ anywhere else.
Technically speaking __ARM_ARMEL__ and __ARM_EABI__ are NOT the same
(__ARM_EABI__ is a more strict specification).
However I doubt the real intention was to make only that piece of code more
generic. 
In any case I don't think we actually support non eabi for arm32, so consistency
wins IMHO.
But that was an *excellent* catch.

https://breakpad.appspot.com/4684002/diff/80001/src/client/linux/minidump_wri...
File src/client/linux/minidump_writer/minidump_writer.cc (left):

https://breakpad.appspot.com/4684002/diff/80001/src/client/linux/minidump_wri...
src/client/linux/minidump_writer/minidump_writer.cc:328: #elif
defined(__ARMEL__)
On 2014/09/30 01:42:58, mmandlis wrote:
> what is the difference (if any) between __ARMEL__ and __ARM_EABI__?
> In the new code, the equivalent of this CPUFillFromThreadInfo function appears
> under #elif defined(__ARM_EABI__)
> in thread_info.cc ln 192

See my reply to your other question.

https://breakpad.appspot.com/4684002/diff/80001/src/client/linux/minidump_wri...
src/client/linux/minidump_writer/minidump_writer.cc:477: #error "This code has
not been ported to your platform yet."
On 2014/09/30 01:42:58, mmandlis wrote:
> I don't think you used this 
> #else
> #error "..." 
> in any of the resulting classes/files. Is this intentional?
I intentionally left it only in raw_context_cpu.h. That is the common include
shared by pretty much everything in this refactoring. I like the idea that when
an error comes up you can easily grep for its source without ambiguities, that's
why I kept it into a single place.

[primiano]

Mark: organizational question: I don't seem to have the powers to commit this
change. How should we deal with this? Can I get the *investiture*? Should I ask
Maria to land it for me?

Maria:
I am amazed by the pedanticity of your review ^__^
Both the #if and the __ARM_EABI__ thing were intentional (see reply inline).
But, if you spotted them and you didn't spot anything else this is very
encouraging for me :)

+nealsid for the question about __ARM_EABI__ vs __ARM_ARMEL__

>and just to be clear, when you're saying that compilers are happy, you also
mean that all tests are still passing, right? 
I tested only on Linux desktop and they pass. I don't have 7 different
tablets/phones with 7 different platforms... yet (I'm getting close).


> btw, should you not be adding the new files to Makefile.am?
Ah absolutely right. I completely forgot about this and cared only about gyp.
Will do that now (before submitting).

[Mark Mentovai]

primiano@chromium.org wrote:

> Mark: organizational question: I don't seem to have the powers to commit
> this change. How should we deal with this? Can I get the *investiture*?
> Should I ask Maria to land it for me?
>

Nope, you’ve got access now.

Maria:
> I am amazed by the pedanticity of your review ^__^
> Both the #if and the __ARM_EABI__ thing were intentional (see reply
> inline).
> But, if you spotted them and you didn't spot anything else this is very
> encouraging for me :)
>
> +nealsid for the question about __ARM_EABI__ vs __ARM_ARMEL__


Neal’s no longer at Google.

 and just to be clear, when you're saying that compilers are happy, you
>>
> also mean that all tests are still passing, right?
> I tested only on Linux desktop and they pass. I don't have 7 different
> tablets/phones with 7 different platforms... yet (I'm getting close).
>
>
>  btw, should you not be adding the new files to Makefile.am?
>>
> Ah absolutely right. I completely forgot about this and cared only about
> gyp. Will do that now (before submitting).
>
>
> https://breakpad.appspot.com/4684002/diff/80001/src/client/
> linux/dump_writer_common/ucontext_reader.cc
> File src/client/linux/dump_writer_common/ucontext_reader.cc (right):
>
> https://breakpad.appspot.com/4684002/diff/80001/src/client/
> linux/dump_writer_common/ucontext_reader.cc#newcode157
> src/client/linux/dump_writer_common/ucontext_reader.cc:157:
> out->context_flags = MD_CONTEXT_ARM_FULL;
> On 2014/09/30 01:42:58, mmandlis wrote:
>
>> same comment - this function appears under
>> #elif defined(__ARM_ARMEL__) in the original code.
>>
>
> Yeah that was intentional. The reason I did it is because that was the
> only instance of __ARM_ARMEL__ in breakpad, which uses __ARM_EABI__
> anywhere else.
> Technically speaking __ARM_ARMEL__ and __ARM_EABI__ are NOT the same
> (__ARM_EABI__ is a more strict specification).
> However I doubt the real intention was to make only that piece of code
> more generic.
> In any case I don't think we actually support non eabi for arm32, so
> consistency wins IMHO.
> But that was an *excellent* catch.
>
> https://breakpad.appspot.com/4684002/diff/80001/src/client/
> linux/minidump_writer/minidump_writer.cc
> File src/client/linux/minidump_writer/minidump_writer.cc (left):
>
> https://breakpad.appspot.com/4684002/diff/80001/src/client/
> linux/minidump_writer/minidump_writer.cc#oldcode328
> src/client/linux/minidump_writer/minidump_writer.cc:328: #elif
> defined(__ARMEL__)
> On 2014/09/30 01:42:58, mmandlis wrote:
>
>> what is the difference (if any) between __ARMEL__ and __ARM_EABI__?
>> In the new code, the equivalent of this CPUFillFromThreadInfo function
>>
> appears
>
>> under #elif defined(__ARM_EABI__)
>> in thread_info.cc ln 192
>>
>
> See my reply to your other question.
>
> https://breakpad.appspot.com/4684002/diff/80001/src/client/
> linux/minidump_writer/minidump_writer.cc#oldcode477
> src/client/linux/minidump_writer/minidump_writer.cc:477: #error "This
> code has not been ported to your platform yet."
> On 2014/09/30 01:42:58, mmandlis wrote:
>
>> I don't think you used this
>> #else
>> #error "..."
>> in any of the resulting classes/files. Is this intentional?
>>
> I intentionally left it only in raw_context_cpu.h. That is the common
> include shared by pretty much everything in this refactoring. I like the
> idea that when an error comes up you can easily grep for its source
> without ambiguities, that's why I kept it into a single place.
>
> https://breakpad.appspot.com/4684002/
>

[primiano]

Committed patchset #3 (id:150002) manually as 1388 (presubmit successful).