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

Issue 9714003: Fix breakpad on mips and x86_64 for the NDK r10c update.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years ago by fdegans
Modified:
2 years, 10 months ago
Base URL:
https://chromium.googlesource.com/external/google-breakpad/src.git@master
Visibility:
Public.

Description

Fix breakpad on mips and x86_64 for the NDK r10c update.

This change introduces the necessary glue typedefs to deal with the
mismatch introduced by the latest Android NDK (w.r.t. desktop Linux):
- [x86_64] Rename fpregs mxcr_mask -> .mxcsr_mask
- [mips] uc_mcontext.fpregs.fp_r.fp_dregs -> uc_mcontext.fpreg
- [mips] restore the forked user.h

TBR=mark

Patch Set 1 #

Total comments: 5

Patch Set 2 : Review comment #

Total comments: 7

Patch Set 3 : Fix indent #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M client/linux/dump_writer_common/thread_info.cc View 1 1 chunk +5 lines, -0 lines 0 comments Download
M client/linux/dump_writer_common/ucontext_reader.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M common/android/include/sys/user.h View 1 2 2 chunks +86 lines, -2 lines 0 comments Download
M common/android/ucontext_constants.h View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 17
fdegans
PTAL I do not include the sys/user.h from the NDK for MIPS, but I do ...
3 years ago #1
primiano
Looks good overall, can you just take a look to the few notes below? https://breakpad.appspot.com/9714003/diff/1/client/linux/dump_writer_common/thread_info.cc ...
3 years ago #2
fdegans
Thanks for the review. I think patch set 2 covers everything. https://breakpad.appspot.com/9714003/diff/1/common/android/include/sys/user.h File common/android/include/sys/user.h (right): ...
3 years ago #3
primiano
LGTM thanks!
3 years ago #4
rmcilroy
https://breakpad.appspot.com/9714003/diff/50001/client/linux/dump_writer_common/thread_info.cc File client/linux/dump_writer_common/thread_info.cc (right): https://breakpad.appspot.com/9714003/diff/50001/client/linux/dump_writer_common/thread_info.cc#newcode183 client/linux/dump_writer_common/thread_info.cc:183: out->flt_save.mx_csr_mask = fpregs.mxcsr_mask; You could do a hacky: #define ...
3 years ago #5
rmcilroy
Actually adding Anton - Anton could you check the x86_64 change please?
3 years ago #6
anton
https://breakpad.appspot.com/9714003/diff/50001/common/android/include/sys/user.h File common/android/include/sys/user.h (right): https://breakpad.appspot.com/9714003/diff/50001/common/android/include/sys/user.h#newcode104 common/android/include/sys/user.h:104: struct user_fpregs_struct { incorrect indentation https://breakpad.appspot.com/9714003/diff/50001/common/android/ucontext_constants.h File common/android/ucontext_constants.h (right): ...
3 years ago #7
fdegans
https://breakpad.appspot.com/9714003/diff/50001/client/linux/dump_writer_common/thread_info.cc File client/linux/dump_writer_common/thread_info.cc (right): https://breakpad.appspot.com/9714003/diff/50001/client/linux/dump_writer_common/thread_info.cc#newcode183 client/linux/dump_writer_common/thread_info.cc:183: out->flt_save.mx_csr_mask = fpregs.mxcsr_mask; On 2014/10/23 14:36:53, rmcilroy wrote: > ...
3 years ago #8
anton
On 2014/10/23 15:43:54, fdegans wrote: > https://breakpad.appspot.com/9714003/diff/50001/client/linux/dump_writer_common/thread_info.cc > File client/linux/dump_writer_common/thread_info.cc (right): > > https://breakpad.appspot.com/9714003/diff/50001/client/linux/dump_writer_common/thread_info.cc#newcode183 > ...
3 years ago #9
fdegans
On 2014/10/23 15:57:42, anton wrote: > > > > > https://breakpad.appspot.com/9714003/diff/50001/common/android/ucontext_constants.h > > File common/android/ucontext_constants.h ...
3 years ago #10
primiano
The short version is that whatever value we put in ucontext_constants.h it's going to be ...
3 years ago #11
primiano
I did some other checks with fresh mind. Confirming that we should NOT rely on ...
3 years ago #12
primiano
Also this line should be removed from common/android/breakpad_getcontext_unittest.cc. ASSERT_EQ(static_cast<size_t>(UCONTEXT_FPREGS_MEM_OFFSET), offsetof(ucontext_t,__fpregs_mem)); This is conceptually wrong, and ...
3 years ago #13
primiano
Ok, another long talk with Anton. Forget my previous two replies. The MCONTEXT_FPREGS_MEM constant is ...
3 years ago #14
anton1
LGTM, with MCONTEXT_FPREGS_MEM set to 304 After more discussion we are retracting some of the ...
3 years ago #15
pasko
Can you guys please make sure that the resolution of the tricky questions like in ...
3 years ago #16
casinobong88
2 years, 10 months ago #17

          
Sign in to reply to this message.

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