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

Issue 49011: Basic arm support for processor (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 4 months ago by Ted Mielczarek
Modified:
14 years, 4 months ago
Reviewers:
Mark Mentovai
CC:
google-breakpad-dev_googlegroups.com
Base URL:
http://google-breakpad.googlecode.com/svn/trunk/
Visibility:
Public.

Description

This patch adds data structures and basic processor support for arm CPUs. Two
caveats to note:
1) The structure definitions do not match Microsoft's structures from the
Windows Mobile SDK. Their structs don't have enough space for VFP3, I'm told,
and I don't think they actually used their structs anywhere anyway, since WinCE
uses a completely different dump format.
2) The stackwalker is basically just stubbed out. AFAIK it can't actually be
written until the DWARF CFI work is done, but Jim Blandy is making progress on
that. Very similar to the x86-64 stackwalker that's not particularly functional
(for the same reason).

I hacked together a test app using the minidump writer classes and produced a
synthetic arm minidump to test with:
http://people.mozilla.com/~tmielczarek/arm.dmp

Mark, could you take a look at this?

Patch Set 1 #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Makefile.am View 4 chunks +5 lines, -0 lines 0 comments Download
src/google_breakpad/common/minidump_cpu_arm.h View 1 chunk +141 lines, -0 lines 5 comments Download
src/google_breakpad/common/minidump_format.h View 1 chunk +4 lines, -4 lines 0 comments Download
src/google_breakpad/processor/minidump.h View 2 chunks +4 lines, -2 lines 1 comment Download
src/google_breakpad/processor/stack_frame_cpu.h View 1 chunk +27 lines, -0 lines 1 comment Download
src/processor/minidump.cc View 8 chunks +113 lines, -2 lines 1 comment Download
src/processor/minidump_processor.cc View 1 chunk +5 lines, -0 lines 0 comments Download
src/processor/stackwalker.cc View 2 chunks +8 lines, -0 lines 0 comments Download
src/processor/stackwalker_arm.cc View 1 chunk +92 lines, -0 lines 1 comment Download
src/processor/stackwalker_arm.h View 1 chunk +80 lines, -0 lines 1 comment Download

Messages

Total messages: 2
Ted Mielczarek
14 years, 4 months ago #1
Mark Mentovai
14 years, 4 months ago #2
LG with some simple changes.

http://breakpad.appspot.com/49011/diff/1/10
File src/google_breakpad/common/minidump_cpu_arm.h (right):

http://breakpad.appspot.com/49011/diff/1/10#newcode41
Line 41: * ensuring that all members are aligned on their natural boundaries. 
In
ensuring ensuring

http://breakpad.appspot.com/49011/diff/1/10#newcode42
Line 42: * In some cases, tail-padding may be significant when different ABIs
specify
In In

http://breakpad.appspot.com/49011/diff/1/10#newcode59
Line 59: * function properly and portably in terms of interoperability with
minidumps
You said that interoperability with ARM minidumps and DbgHelp is actually not a
concern here.

http://breakpad.appspot.com/49011/diff/1/10#newcode128
Line 128: } MDRawContextARM;  /* CONTEXT_ARM */
Dunno if we need to provide the comments that say what the Microsoft names for
these are, since we don't seek compatibility with any Microsoft-defined structs.

http://breakpad.appspot.com/49011/diff/1/10#newcode141
Line 141: #endif /* GOOGLE_BREAKPAD_COMMON_MINIDUMP_CPU_ARM_H__ */
Two spaces before the comment:

#endif  /* GOOGLE_BREAKPAD_COMMON_MINIDUMP_CPU_ARM_H__ */

http://breakpad.appspot.com/49011/diff/1/7
File src/google_breakpad/processor/minidump.h (right):

http://breakpad.appspot.com/49011/diff/1/7#newcode214
Line 214: MDRawContextBase*  base;
Looks like the union fields were once lined up vertically.  We should either
continue to line them up, or not align any of them.

http://breakpad.appspot.com/49011/diff/1/8
File src/google_breakpad/processor/stack_frame_cpu.h (right):

http://breakpad.appspot.com/49011/diff/1/8#newcode181
Line 181: CONTEXT_VALID_ALL  = -1
Nuke extra space before =.

http://breakpad.appspot.com/49011/diff/1/4
File src/processor/minidump.cc (right):

http://breakpad.appspot.com/49011/diff/1/4#newcode2833
Line 2833: // Don't log as an error if we can still fall back on the thread's
context
:)

http://breakpad.appspot.com/49011/diff/1/5
File src/processor/stackwalker_arm.cc (right):

http://breakpad.appspot.com/49011/diff/1/5#newcode86
Line 86: //TODO: We can't actually walk the stack on ARM without the CFI data.
Nit: space between // and TODO.

Nit: don't use "we."

http://breakpad.appspot.com/49011/diff/1/3
File src/processor/stackwalker_arm.h (right):

http://breakpad.appspot.com/49011/diff/1/3#newcode65
Line 65: //TODO: currently stubbed out, needs CFI symbol dumper support
// TODO:
Sign in to reply to this message.

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