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

Issue 3744002: [MIPS]: Use mcontext_t structure for MIPS (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 8 months ago by gordanac
Modified:
6 years, 3 months ago
CC:
Petar Jovanovic, paul.lind_imgtec.com
Base URL:
http://google-breakpad.googlecode.com/svn/trunk/src/
Visibility:
Public.

Description

[MIPS]: Use mcontext_t structure for MIPS

This change removes user_regs_struct and
user_fpregs_struct structures for mips
and uses mcontext_t instead.

Patch Set 1 #

Patch Set 2 : Put back register defines to user.h #

Total comments: 10

Patch Set 3 : Addressing first comments #

Patch Set 4 : In case of Android use register definitions from asm/reg.h #

Total comments: 5

Patch Set 5 : Modify register getters in thread_info #

Patch Set 6 : Correct spelling error. #

Total comments: 1

Patch Set 7 : Use seperate variable for regs and fregs in LinuxPtraceDumper::GetThreadInfoByIndex() #

Patch Set 8 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M client/linux/dump_writer_common/thread_info.cc View 1 2 3 4 5 6 7 3 chunks +49 lines, -16 lines 0 comments Download
M client/linux/dump_writer_common/thread_info.h View 1 2 3 4 5 6 7 2 chunks +8 lines, -5 lines 0 comments Download
M client/linux/minidump_writer/linux_core_dumper.cc View 1 2 3 4 5 6 7 3 chunks +16 lines, -11 lines 0 comments Download
M client/linux/minidump_writer/linux_ptrace_dumper.cc View 1 2 3 4 5 6 7 3 chunks +23 lines, -15 lines 0 comments Download
M client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M common/android/include/sys/procfs.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M common/android/include/sys/user.h View 1 2 3 4 5 6 7 3 chunks +0 lines, -78 lines 0 comments Download
M google_breakpad/common/minidump_cpu_mips.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -20 lines 0 comments Download
M tools/linux/md2core/minidump-2-core.cc View 1 2 3 4 5 6 7 4 chunks +32 lines, -14 lines 0 comments Download

Messages

Total messages: 16
fdegans
non-owner lgtm, just one comment. https://breakpad.appspot.com/3744002/diff/30001/common/android/include/sys/user.h File common/android/include/sys/user.h (right): https://breakpad.appspot.com/3744002/diff/30001/common/android/include/sys/user.h#newcode37 common/android/include/sys/user.h:37: // in NDK but ...
6 years, 3 months ago #1
rmcilroy
Thanks for the fixes, some comments below. https://breakpad.appspot.com/3744002/diff/30001/client/linux/dump_writer_common/thread_info.cc File client/linux/dump_writer_common/thread_info.cc (right): https://breakpad.appspot.com/3744002/diff/30001/client/linux/dump_writer_common/thread_info.cc#newcode246 client/linux/dump_writer_common/thread_info.cc:246: out->mdhi = ...
6 years, 3 months ago #2
gordanac
Hi Ross, Please take a look into new patchsets: Patchset #3 includes code rebase as ...
6 years, 3 months ago #3
rmcilroy
Looking a lot better, a few more comments. https://breakpad.appspot.com/3744002/diff/240001/client/linux/dump_writer_common/thread_info.cc File client/linux/dump_writer_common/thread_info.cc (right): https://breakpad.appspot.com/3744002/diff/240001/client/linux/dump_writer_common/thread_info.cc#newcode266 client/linux/dump_writer_common/thread_info.cc:266: // ...
6 years, 3 months ago #4
gordanac
Hi Ross, New patchset is uploaded with comments: https://breakpad.appspot.com/3744002/diff/240001/client/linux/dump_writer_com...<https://breakpad.appspot.com/3744002/diff/240001/client/linux/dump_writer_common/thread_info.cc> File client/linux/dump_writer_common/thread_info.cc (right): https://breakpad.appspot.com/3744002/diff/240001/client/linux/dump_writer_com...<https://breakpad.appspot.com/3744002/diff/240001/client/linux/dump_writer_common/thread_info.cc#newcode266> client/linux/dump_writer_common/thread_info.cc:266: // ...
6 years, 3 months ago #5
gordanac
Hi Ross, In case https://breakpad.appspot.com/6824002/ gets merged I will have to rebase this one since ...
6 years, 3 months ago #6
rmcilroy
One last comment but then lgtm. https://breakpad.appspot.com/3744002/diff/350001/client/linux/minidump_writer/linux_ptrace_dumper.cc File client/linux/minidump_writer/linux_ptrace_dumper.cc (right): https://breakpad.appspot.com/3744002/diff/350001/client/linux/minidump_writer/linux_ptrace_dumper.cc#newcode203 client/linux/minidump_writer/linux_ptrace_dumper.cc:203: void* addr; have ...
6 years, 3 months ago #7
gordanac
https://breakpad.appspot.com/3744002/diff/350001/client/linux/minidump_writer...<https://breakpad.appspot.com/3744002/diff/350001/client/linux/minidump_writer/linux_ptrace_dumper.cc> File client/linux/minidump_writer/linux_ptrace_dumper.cc (right): https://breakpad.appspot.com/3744002/diff/350001/client/linux/minidump_writer...<https://breakpad.appspot.com/3744002/diff/350001/client/linux/minidump_writer/linux_ptrace_dumper.cc#newcode203> client/linux/minidump_writer/linux_ptrace_dumper.cc:203: void* addr; On 2015/04/10 16:56:30, rmcilroy wrote: > have ...
6 years, 3 months ago #8
rmcilroy
lgtm, thanks. Did you test this doens't break anything on the other architectures?
6 years, 3 months ago #9
gordanac
Tests run (beside mipsel): Breakpads's make check for x86-64 Linux and Chromium's breakpad_unittests for ARM ...
6 years, 3 months ago #10
rmcilroy
On 2015/04/15 13:58:37, gordanac wrote: > Tests run (beside mipsel): Breakpads's make check for x86-64 ...
6 years, 3 months ago #11
gordanac
Lets wait till tomorrow. ________________________________________ From: rmcilroy@chromium.org [rmcilroy@chromium.org] Sent: Wednesday, April 15, 2015 4:33 PM ...
6 years, 3 months ago #12
gordanac
Rebased. New patch set uploaded. ________________________________________ From: Gordana Cmiljanovic Sent: Wednesday, April 15, 2015 6:34 ...
6 years, 3 months ago #13
rmcilroy
lgtm, thanks. Mark: do you want a look before I land this?
6 years, 3 months ago #14
Mark Mentovai
LGTM
6 years, 3 months ago #15
rmcilroy
6 years, 3 months ago #16
On 2015/04/21 13:25:02, Mark Mentovai wrote:
> LGTM

Landed as r1452 [1].  I can't close this CL myself so you will need to do that
manually. Thanks.

[1] https://code.google.com/p/google-breakpad/source/detail?r=1452
Sign in to reply to this message.

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