Use frame pointer to walk ARM stack on iOS.

Use frame pointer to walk ARM stack on iOS.

[Ted Mielczarek]

Is there a reason you can't just use the DWARF CFI to walk stacks from iOS? Is
this just to get you out of system library frames?

[qsr]

On 2011/10/14 12:00:02, Ted Mielczarek wrote:
> Is there a reason you can't just use the DWARF CFI to walk stacks from iOS? Is
> this just to get you out of system library frames?

 1) I cannot get CFI for any system library, so yes, this helps a lot.
 2) CFI just doesn't seem to work on iOS right now. I do not know exactly why
(and I do not know enough of CFI right now), but:
  * iOS use DW_CFA_offset_extended: on its CFI, and I cannot find what that can
possibily means as arm has only 37 register, which fits perfectly on 6 bits (it
uses the offset extended with register number something like 272 to 286 by step
of 2)
  * parsing of the CFI file that I have is failing -> the parser seems to
consider that he must pop from an empty stack, then my register are not register
anymore, and all fails.

[Mark Mentovai]

Let’s hang on to this for a sec until you’ve debugged the “not seeing the most
important frame” thing a little bit more.

http://breakpad.appspot.com/314001/diff/3001/8
File src/processor/stackwalker_arm.cc (right):

http://breakpad.appspot.com/314001/diff/3001/8#newcode259
Line 259: frame->instruction = frame->context.iregs[MD_CONTEXT_ARM_REG_PC] - 2;
Because thumb instructions are two bytes?

[qsr]

On 2011/10/14 14:00:50, Mark Mentovai wrote:
> Let’s hang on to this for a sec until you’ve debugged the “not seeing the most
> important frame” thing a little bit more.

 Ok.

> http://breakpad.appspot.com/314001/diff/3001/8
> File src/processor/stackwalker_arm.cc (right):
> 
> http://breakpad.appspot.com/314001/diff/3001/8#newcode259
> Line 259: frame->instruction = frame->context.iregs[MD_CONTEXT_ARM_REG_PC] -
2;
> Because thumb instructions are two bytes?

 Yes, and because when only decrementing by 1, the stack walker find that all
instruction are exactly the next instruction to be executed and all line number
are wrong by 1.

[qsr]

PTAL

[Mark Mentovai]

http://breakpad.appspot.com/314001/diff/4004/2006
File src/processor/stackwalker_arm.cc (right):

http://breakpad.appspot.com/314001/diff/4004/2006#newcode187
Line 187: StackFrameARM *frame = new StackFrameARM();
This is what you encounter at the end of a walk, right? Why not just return NULL
here to signal the end? What you’re returning now is a totally bogus frame.

http://breakpad.appspot.com/314001/diff/4004/2006#newcode201
Line 201: u_int32_t caller_lr;
Don’t declare this ’til you use it (below), but I’m still not sure it’s right.

http://breakpad.appspot.com/314001/diff/4004/2006#newcode215
Line 215: if (!memory_->GetMemoryAtAddress(caller_fp + 4, &caller_lr)) {
Is this right?

In the context frame, we take the lr as-is. That is, in that frame, we use the
value of lr as it exists at the crash site.

In the context frame’s caller, don’t we want to recover the lr that is saved in
the context frame? That would be at crash_fp + 4. I don’t see where else you
ever recover anything from crash_fp + 4. The walk seems to skip over that lr
value.

I think this may have something to do with your having to maintain two separate
blocks to create a new frame, the |caller_fp == 0| one above and the
“normal” one below.

http://breakpad.appspot.com/314001/diff/4004/2006#newcode235
Line 235: StackFrameARM::CONTEXT_VALID_SP;
FP is valid too, right? For all of these, actually.

[jimb]

These changes need tests. stackwalker_arm_unittest.cc should be capable of
reliably exercising the new code.

http://breakpad.appspot.com/314001/diff/4004/2006
File src/processor/stackwalker_arm.cc (right):

http://breakpad.appspot.com/314001/diff/4004/2006#newcode261
Line 261: frame.reset(GetCallerByStackScan(frames));
If we have an fp_register, but GetCallerByFramePointer fails, does it make sense
to try GetCallerByStackStackScan?

That is, the structure of the code here is as a series of strategies, ordered
from most reliable to least reliable. If I understand correctly,
GetCallerByFramePointer falls between CFI and stack scanning. So it should read
something like:

if (!frame.get() && fp_register_ >= 0)
  frame.reset(GetCallerByFramePointer(frames))

if (!frame.get())
  frame.reset(GetCallerByStackScan);

[qsr]

http://breakpad.appspot.com/314001/diff/4004/2006
File src/processor/stackwalker_arm.cc (right):

http://breakpad.appspot.com/314001/diff/4004/2006#newcode235
Line 235: StackFrameARM::CONTEXT_VALID_SP;
On 2011/10/14 16:34:31, Mark Mentovai wrote:
> FP is valid too, right? For all of these, actually.

 Yes, but setting it breaks the code, because the CFI section then starts to
work, but seems incorrect. Here a CFI section I have:


STACK CFI INIT 7c023c 14 .cfa: sp 0 + .ra: lr
STACK CFI 7c023e .cfa: sp 8 + .ra: .cfa -4 + ^ r7: .cfa -8 + ^
STACK CFI 7c0240 .cfa: r7 8 +

 The important point is that this is reading lr, but never setting it back. I
can probably make it works by taking into account myself the interaction between
lr and pc, but I do not know how linux on arm works and I really fear to break
something.

[qsr]

PTAL.

 To be noted:

 Thanks for pointing out my error with the FP. The code was totally wrong
because of that. I did rewrite the walk, and it looks a lot nicer now.
 Just setting the fp_register as valid break the walk on iOS. This is because
the CFI is using .ra to designate CF, while the CFI code used it for PC. I
handle the 2 cases differently now, and I added a test that check that mixing
both works as expected.
 I did write tests for the iOS case.

http://breakpad.appspot.com/314001/diff/4004/2006
File src/processor/stackwalker_arm.cc (right):

http://breakpad.appspot.com/314001/diff/4004/2006#newcode215
Line 215: if (!memory_->GetMemoryAtAddress(caller_fp + 4, &caller_lr)) {
On 2011/10/14 16:34:31, Mark Mentovai wrote:
> Is this right?
> 
> In the context frame, we take the lr as-is. That is, in that frame, we use the
> value of lr as it exists at the crash site.
> 
> In the context frame’s caller, don’t we want to recover the lr that is saved
in
> the context frame? That would be at crash_fp + 4. I don’t see where else you
> ever recover anything from crash_fp + 4. The walk seems to skip over that lr
> value.
> 
> I think this may have something to do with your having to maintain two
separate
> blocks to create a new frame, the |caller_fp == 0| one above and the
> “normal” one below.

Done.

http://breakpad.appspot.com/314001/diff/4004/2006#newcode235
Line 235: StackFrameARM::CONTEXT_VALID_SP;
On 2011/10/14 16:34:31, Mark Mentovai wrote:
> FP is valid too, right? For all of these, actually.

Done.

http://breakpad.appspot.com/314001/diff/4004/2006#newcode261
Line 261: frame.reset(GetCallerByStackScan(frames));
On 2011/10/14 17:17:23, jimb wrote:
> If we have an fp_register, but GetCallerByFramePointer fails, does it make
sense
> to try GetCallerByStackStackScan?
> 
> That is, the structure of the code here is as a series of strategies, ordered
> from most reliable to least reliable. If I understand correctly,
> GetCallerByFramePointer falls between CFI and stack scanning. So it should
read
> something like:
> 
> if (!frame.get() && fp_register_ >= 0)
>   frame.reset(GetCallerByFramePointer(frames))
> 
> if (!frame.get())
>   frame.reset(GetCallerByStackScan);
> 
> 

Done.

[Mark Mentovai]

I’m a little bothered that based on what you’ve said, the iOS CFI is pretty much
unusable because it doesn’t recover LR. I think we should figure that out,
although I’m personally also OK with committing this as-is now as long as you
move on to handling the iOS CFI thing immediately afterwards. It’s possible that
the ARM dumper for iOS just needs to output instructions for recovering lr, or
it’s possible that the ARM CFI walker needs to recover lr even when the postfix
instructions don’t tell it to do it explicitly. I’d be interested in how GDB or
other CFI users handle this on ARM.

I think that not handling the CFI stuff will have undesired consequences in the
short term as it’s likely that our stacks will have a mix of code for which we
have and don’t have CFI, and if we’re not recovering lr when we’re in CFI code,
it can have implications when the backtrace winds up back in code that we try to
do FP recovery for.

That is: LGTM if you want to check this in now as-is as long as the CFI stuff
doesn’t remain buried under a rock.

http://breakpad.appspot.com/314001/diff/9001/2018
File src/processor/stackwalker_arm.cc (right):

http://breakpad.appspot.com/314001/diff/9001/2018#newcode132
Line 132: // The CFI update the link register and not the program counter.
update -> updated?

http://breakpad.appspot.com/314001/diff/9001/2018#newcode197
Line 197: StackFrameARM::RegisterValidFlag(fp_register_)))
Use {braces} because the condition is spread across multiple lines.

http://breakpad.appspot.com/314001/diff/9001/2018#newcode211
Line 211: BPLOG(ERROR) << "Unable to read caller_lr from last_fp: 0x"
This message might be more useful if it stated last_fp + 4 and printed last_fp +
4.

[Mark Mentovai]

I misread some of qsr’s comments. This is LGTM as-is.