[mips64] Support for mips n64

[mips64] Support for mips n64

Adding remaining mips n64 support including stackwalker.

BUG=None
TEST=manually tested on Linux/Android
R=vapier@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/79556b7ca86baf84cbd29f69e7968f8e7a722f78

[mveljko, 2015-11-09 13:49:53]

Description was changed from

==========
[mips64] Support for mips64

Adding remaining mips64 support including stackwalker.

BUG=None
TEST=manually tested on Linux/Android
==========

to

==========
[mips64] Support for mips64

Adding remaining mips64 support including stackwalker.

BUG=None
TEST=manually tested on Linux/Android
==========

[mveljko, 2015-11-09 13:49:53]

veljko.mihailovic@imgtec.com changed reviewers:
+ mark@chromium.org

[mveljko, 2015-11-09 13:50:46]

Please, take a look at the changes.

[mveljko, 2015-11-18 09:52:25]

On 2015/11/09 13:50:46, mveljko wrote:
> Please, take a look at the changes.

Just a friendly ping.

[Mark Mentovai, 2015-11-18 14:30:49]

I don’t think I’m going to be able to review this in the next couple of weeks.

[Mark Mentovai, 2015-11-18 14:31:38]

Have your colleagues like Gordana that have submitted MIPS patches in the past
had a chance to look at this yet?

[gordanac, 2015-11-18 15:49:01]

On 2015/11/18 14:31:38, Mark Mentovai wrote:
> Have your colleagues like Gordana that have submitted MIPS patches in the past
> had a chance to look at this yet?

The change is OK on my side.

Could you recommend another reviewer if you don't have time to do it?

[Mark Mentovai, 2015-11-18 15:55:11]

We could open it up to any interested committer on google-breakpad-dev.

[mveljko, 2015-11-26 15:54:28]

veljko.mihailovic@imgtec.com changed reviewers:
+ ivan.penkov@gmail.com

[vapier, 2015-12-15 17:29:26]

vapier@chromium.org changed reviewers:
+ vapier@chromium.org

[vapier, 2015-12-15 17:29:27]

please clarify the commit message that you're adding support only for o64 here
and not "mips64" in general (i.e. this doesn't support n32)

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/dump_write...
File src/client/linux/dump_writer_common/thread_info.cc (right):

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/dump_write...
src/client/linux/dump_writer_common/thread_info.cc:240: #else
probably want to do here (and in other places in this patch like this):

#elif _MIPS_SIM == _ABIO32

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/microdump_...
File src/client/linux/microdump_writer/microdump_writer.cc (right):

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/microdump_...
src/client/linux/microdump_writer/microdump_writer.cc:181: #elif
defined(__mips__) && _MIPS_SIM == _ABIO32
would be cleaner imo to write:

#if defined(__mips__)
# if _MIPS_SIM == _ABIO32
    const char kArch[] = "mips";
# elif _MIPS_SIM == _ABI64
    const char kArch[] = "mips64";
# endif
#else

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/minidump_w...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/minidump_writer.cc:822: #if defined(__mips__)
&& _MIPS_SIM == _ABIO32
same comment here

https://codereview.chromium.org/1418453011/diff/1/src/tools/linux/md2core/min...
File src/tools/linux/md2core/minidump-2-core.cc (right):

https://codereview.chromium.org/1418453011/diff/1/src/tools/linux/md2core/min...
src/tools/linux/md2core/minidump-2-core.cc:499: "This version of minidump-2-core
only supports mips (32bit).\n");
would be nice imo to put "o32" in here

https://codereview.chromium.org/1418453011/diff/1/src/tools/linux/md2core/min...
src/tools/linux/md2core/minidump-2-core.cc:505: "This version of minidump-2-core
only supports mips (64bit).\n");
would be nice imo to put "o64" in here

[mveljko, 2015-12-17 13:36:25]

Updated code, based on comments.

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/dump_write...
File src/client/linux/dump_writer_common/thread_info.cc (right):

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/dump_write...
src/client/linux/dump_writer_common/thread_info.cc:240: #else
On 2015/12/15 17:29:27, vapier wrote:
> probably want to do here (and in other places in this patch like this):
> 
> #elif _MIPS_SIM == _ABIO32

Done.

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/microdump_...
File src/client/linux/microdump_writer/microdump_writer.cc (right):

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/microdump_...
src/client/linux/microdump_writer/microdump_writer.cc:181: #elif
defined(__mips__) && _MIPS_SIM == _ABIO32
On 2015/12/15 17:29:27, vapier wrote:
> would be cleaner imo to write:
> 
> #if defined(__mips__)
> # if _MIPS_SIM == _ABIO32
>     const char kArch[] = "mips";
> # elif _MIPS_SIM == _ABI64
>     const char kArch[] = "mips64";
> # endif
> #else

Done.

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/minidump_w...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://codereview.chromium.org/1418453011/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/minidump_writer.cc:822: #if defined(__mips__)
&& _MIPS_SIM == _ABIO32
On 2015/12/15 17:29:27, vapier wrote:
> same comment here

Done.

https://codereview.chromium.org/1418453011/diff/1/src/tools/linux/md2core/min...
File src/tools/linux/md2core/minidump-2-core.cc (right):

https://codereview.chromium.org/1418453011/diff/1/src/tools/linux/md2core/min...
src/tools/linux/md2core/minidump-2-core.cc:499: "This version of minidump-2-core
only supports mips (32bit).\n");
On 2015/12/15 17:29:27, vapier wrote:
> would be nice imo to put "o32" in here

Done.

https://codereview.chromium.org/1418453011/diff/1/src/tools/linux/md2core/min...
src/tools/linux/md2core/minidump-2-core.cc:505: "This version of minidump-2-core
only supports mips (64bit).\n");
On 2015/12/15 17:29:27, vapier wrote:
> would be nice imo to put "o64" in here

Done.

[mveljko, 2015-12-17 13:37:13]

Description was changed from

==========
[mips64] Support for mips64

Adding remaining mips64 support including stackwalker.

BUG=None
TEST=manually tested on Linux/Android
==========

to

==========
[mips64] Support for mips n64

Adding remaining mips n64 support including stackwalker.

BUG=None
TEST=manually tested on Linux/Android
==========

[mveljko, 2016-02-01 10:02:25]

On 2015/12/17 13:36:25, mveljko wrote:
> Updated code, based on comments.
> 
>
https://codereview.chromium.org/1418453011/diff/1/src/client/linux/dump_write...
> File src/client/linux/dump_writer_common/thread_info.cc (right):
> 
>
https://codereview.chromium.org/1418453011/diff/1/src/client/linux/dump_write...
> src/client/linux/dump_writer_common/thread_info.cc:240: #else
> On 2015/12/15 17:29:27, vapier wrote:
> > probably want to do here (and in other places in this patch like this):
> > 
> > #elif _MIPS_SIM == _ABIO32
> 
> Done.
> 
>
https://codereview.chromium.org/1418453011/diff/1/src/client/linux/microdump_...
> File src/client/linux/microdump_writer/microdump_writer.cc (right):
> 
>
https://codereview.chromium.org/1418453011/diff/1/src/client/linux/microdump_...
> src/client/linux/microdump_writer/microdump_writer.cc:181: #elif
> defined(__mips__) && _MIPS_SIM == _ABIO32
> On 2015/12/15 17:29:27, vapier wrote:
> > would be cleaner imo to write:
> > 
> > #if defined(__mips__)
> > # if _MIPS_SIM == _ABIO32
> >     const char kArch[] = "mips";
> > # elif _MIPS_SIM == _ABI64
> >     const char kArch[] = "mips64";
> > # endif
> > #else
> 
> Done.
> 
>
https://codereview.chromium.org/1418453011/diff/1/src/client/linux/minidump_w...
> File src/client/linux/minidump_writer/minidump_writer.cc (right):
> 
>
https://codereview.chromium.org/1418453011/diff/1/src/client/linux/minidump_w...
> src/client/linux/minidump_writer/minidump_writer.cc:822: #if defined(__mips__)
> && _MIPS_SIM == _ABIO32
> On 2015/12/15 17:29:27, vapier wrote:
> > same comment here
> 
> Done.
> 
>
https://codereview.chromium.org/1418453011/diff/1/src/tools/linux/md2core/min...
> File src/tools/linux/md2core/minidump-2-core.cc (right):
> 
>
https://codereview.chromium.org/1418453011/diff/1/src/tools/linux/md2core/min...
> src/tools/linux/md2core/minidump-2-core.cc:499: "This version of
minidump-2-core
> only supports mips (32bit).\n");
> On 2015/12/15 17:29:27, vapier wrote:
> > would be nice imo to put "o32" in here
> 
> Done.
> 
>
https://codereview.chromium.org/1418453011/diff/1/src/tools/linux/md2core/min...
> src/tools/linux/md2core/minidump-2-core.cc:505: "This version of
minidump-2-core
> only supports mips (64bit).\n");
> On 2015/12/15 17:29:27, vapier wrote:
> > would be nice imo to put "o64" in here
> 
> Done.

Just a friendly reminder.

[vapier, 2016-02-01 22:43:42]

i've lightly glanced at the mips-specific parts and nothing catches my eye. 
i'll defer to you as an imgtec employee ;).

have you verified that unittests still pass for mips32 ?

https://codereview.chromium.org/1418453011/diff/20001/Makefile.am
File Makefile.am (right):

https://codereview.chromium.org/1418453011/diff/20001/Makefile.am#newcode1152
Makefile.am:1152: src/testing/gtest/src/gtest-all.cc \
you will need to update this after recent changes.  see commit
83f0e6a31cd7e86e36a2a33085e86f8cc2a6fa7b for details.

https://codereview.chromium.org/1418453011/diff/20001/Makefile.am#newcode1158
Makefile.am:1158: src_processor_stackwalker_mips64_unittest_CPPFLAGS = \
you will need to update this after recent changes.  see commit
72a02f6cbff3a84587ec83c455482f06a4a09faa for details.

[mveljko, 2016-02-04 14:32:24]

https://codereview.chromium.org/1418453011/diff/20001/Makefile.am
File Makefile.am (right):

https://codereview.chromium.org/1418453011/diff/20001/Makefile.am#newcode1152
Makefile.am:1152: src/testing/gtest/src/gtest-all.cc \
On 2016/02/01 22:43:42, vapier wrote:
> you will need to update this after recent changes.  see commit
> 83f0e6a31cd7e86e36a2a33085e86f8cc2a6fa7b for details.

Done.

https://codereview.chromium.org/1418453011/diff/20001/Makefile.am#newcode1158
Makefile.am:1158: src_processor_stackwalker_mips64_unittest_CPPFLAGS = \
On 2016/02/01 22:43:42, vapier wrote:
> you will need to update this after recent changes.  see commit
> 72a02f6cbff3a84587ec83c455482f06a4a09faa for details.

Done.

[mveljko, 2016-02-04 14:47:46]

On 2016/02/01 22:43:42, vapier wrote:
> i've lightly glanced at the mips-specific parts and nothing catches my eye. 
> i'll defer to you as an imgtec employee ;).
> 
> have you verified that unittests still pass for mips32 ?
Yes, I have and all tests are passing on mips32 and mips64.

[vapier, 2016-02-04 18:30:59]

https://codereview.chromium.org/1418453011/diff/60001/Makefile.am
File Makefile.am (right):

https://codereview.chromium.org/1418453011/diff/60001/Makefile.am#newcode1059
Makefile.am:1059: src/testing/gtest/src/gtest-all.cc \
doesn't seem like you updated properly ... please see commit
83f0e6a31cd7e86e36a2a33085e86f8cc2a6fa7b

[mveljko, 2016-02-05 09:45:32]

https://codereview.chromium.org/1418453011/diff/60001/Makefile.am
File Makefile.am (right):

https://codereview.chromium.org/1418453011/diff/60001/Makefile.am#newcode1059
Makefile.am:1059: src/testing/gtest/src/gtest-all.cc \
On 2016/02/04 18:30:59, vapier wrote:
> doesn't seem like you updated properly ... please see commit
> 83f0e6a31cd7e86e36a2a33085e86f8cc2a6fa7b

Done.

[mveljko, 2016-02-05 09:46:25]

On 2016/02/04 18:30:59, vapier wrote:
> https://codereview.chromium.org/1418453011/diff/60001/Makefile.am
> File Makefile.am (right):
> 
> https://codereview.chromium.org/1418453011/diff/60001/Makefile.am#newcode1059
> Makefile.am:1059: src/testing/gtest/src/gtest-all.cc \
> doesn't seem like you updated properly ... please see commit
> 83f0e6a31cd7e86e36a2a33085e86f8cc2a6fa7b

My bad. I fixed it.

[vapier, 2016-02-05 17:52:24]

lgtm

[vapier, 2016-02-06 23:58:44]

Description was changed from

==========
[mips64] Support for mips n64

Adding remaining mips n64 support including stackwalker.

BUG=None
TEST=manually tested on Linux/Android
==========

to

==========
[mips64] Support for mips n64

Adding remaining mips n64 support including stackwalker.

BUG=None
TEST=manually tested on Linux/Android
R=vapier@chromium.org

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/79556b7ca86baf84cbd29f6...
==========

[vapier, 2016-02-06 23:58:45]

Committed patchset #6 (id:100001) manually as
79556b7ca86baf84cbd29f69e7968f8e7a722f78 (presubmit successful).