Off by one error in EIP/RIP addresses on x86/x64

Off by one error in EIP/RIP addresses on x86/x64

[bruce.dawson]

In StackwalkerX86::GetCallerFrame() an offset of minus one is applied to EIP in
order to make symbol lookup work in a particular way. That's fine. The comment
says "Callers that require the exact return address value may access the
context.eipfield of StackFrameX86." So far so good.

However in PrintStack() this advice is forgotten. frame->instruction is printed
without corrections. This means that any addresses or offsets printed (with the
exception of frame 0) are off by one. This caused some confusion when we were
investigating a crash in an area without symbols. We eventually figured out the
need to add one, but obviously this is not ideal.

This issue occurs on amd64 as well. Based on code inspection it does not appear
to happen with other processors.

This code fix corrects some addresses used in the tests so I also updated the
expected test outputs.

[jimb]

Thanks for catching this! I think your diagnosis of the problem is right on the
mark.

The rationale for the -1 is that the return address may point to the first
instruction of the next line, whereas people want the backtrace to show the
call. If the call is to a function that the compiler knew would never return,
the compiler may have placed that call at the very end of the instruction, so
that the 'return address' (which will never be used) points into an entirely
different function, or perhaps a different file!

Essentially, what you want for looking up source positions is the address of the
call, not the address of the return, but StackFrame completely fails to
recognize this distinction. Ideally, instead of 'instruction' we'd have
'return_address', and 'CallAddress' would be an inline function that just
subtracts one from return_address (with an explanation of why), and pretty much
all current uses of 'instruction' would use StackFrame::CallAddress instead.

That would be a pretty big change, though. I don't think it's fair to ask you to
do it in this bug.

I'm not fond of putting per-processor knowledge about how to get the true
instruction address, in the printing function. I would rather see the hair
isolated in the architecture-specific code that needs it.

One alternative would be to add a virtual method 'ReturnAddress', to StackFrame,
with a definition in the base class that simply returns 'instruction'.
StackFrameX86, etc. could override that to undo the offset that their unwinders
apply to StackFrame::instruction. And then PrintStack could call
StackFrame::ReturnAddress to get the right thing on all architectures.

http://breakpad.appspot.com/514002/diff/1/src/processor/minidump_stackwalk.cc
File src/processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/514002/diff/1/src/processor/minidump_stackwalk.cc...
src/processor/minidump_stackwalk.cc:148: // StackwalkerAMD64::GetCallerFrame)
subtracts one from the instruction pointer
Does the inserted code use tab characters here? It's Google style to use only
space characters.

[bruce.dawson]

Oops -- some tabs snuck in. I've fixed those in my copy. I'll look into the 
virtual function fix.

On Friday, January 18, 2013 5:26:41 PM UTC-8, Jim Blandy wrote:

> Thanks for catching this! I think your diagnosis of the problem is right 
> on the mark. 
>
> The rationale for the -1 is that the return address may point to the 
> first instruction of the next line, whereas people want the backtrace to 
> show the call. If the call is to a function that the compiler knew would 
> never return, the compiler may have placed that call at the very end of 
> the instruction, so that the 'return address' (which will never be used) 
> points into an entirely different function, or perhaps a different file! 
>
> Essentially, what you want for looking up source positions is the 
> address of the call, not the address of the return, but StackFrame 
> completely fails to recognize this distinction. Ideally, instead of 
> 'instruction' we'd have 'return_address', and 'CallAddress' would be an 
> inline function that just subtracts one from return_address (with an 
> explanation of why), and pretty much all current uses of 'instruction' 
> would use StackFrame::CallAddress instead. 
>
> That would be a pretty big change, though. I don't think it's fair to 
> ask you to do it in this bug. 
>
> I'm not fond of putting per-processor knowledge about how to get the 
> true instruction address, in the printing function. I would rather see 
> the hair isolated in the architecture-specific code that needs it. 
>
> One alternative would be to add a virtual method 'ReturnAddress', to 
> StackFrame, with a definition in the base class that simply returns 
> 'instruction'. StackFrameX86, etc. could override that to undo the 
> offset that their unwinders apply to StackFrame::instruction. And then 
> PrintStack could call StackFrame::ReturnAddress to get the right thing 
> on all architectures. 
>
>
>
> http://breakpad.appspot.com/514002/diff/1/src/processor/minidump_stackwalk.cc

> File src/processor/minidump_stackwalk.cc (right): 
>
>
http://breakpad.appspot.com/514002/diff/1/src/processor/minidump_stackwalk.cc...

>
>
src/processor/minidump_stackwalk.cc:148<http://breakpad.appspot.com/514002/diff/1/src/processor/minidump_stackwalk.cc#newcode148src/processor/minidump_stackwalk.cc:148>:

> // 
> StackwalkerAMD64::GetCallerFrame) subtracts one from the instruction 
> pointer 
> Does the inserted code use tab characters here? It's Google style to use 
> only space characters. 
>
> http://breakpad.appspot.com/514002/ 
>

[bruce.dawson]

I updated it to use a virtual function -- that does clean things up a bit.
 
The implementation is totally straightforward. Some of the comments are a 
bit redundant, and the if checks in the two derived implementations of 
ReturnAddress() are not really needed -- any thoughts/preferences on 
whether to delete the repeated comments and unnecessary if checks?

On Monday, January 21, 2013 2:47:40 PM UTC-8, bruce....@gmail.com wrote:

> Oops -- some tabs snuck in. I've fixed those in my copy. I'll look into 
> the virtual function fix.
>
> On Friday, January 18, 2013 5:26:41 PM UTC-8, Jim Blandy wrote:
>
>> Thanks for catching this! I think your diagnosis of the problem is right 
>> on the mark. 
>>
>> The rationale for the -1 is that the return address may point to the 
>> first instruction of the next line, whereas people want the backtrace to 
>> show the call. If the call is to a function that the compiler knew would 
>> never return, the compiler may have placed that call at the very end of 
>> the instruction, so that the 'return address' (which will never be used) 
>> points into an entirely different function, or perhaps a different file! 
>>
>> Essentially, what you want for looking up source positions is the 
>> address of the call, not the address of the return, but StackFrame 
>> completely fails to recognize this distinction. Ideally, instead of 
>> 'instruction' we'd have 'return_address', and 'CallAddress' would be an 
>> inline function that just subtracts one from return_address (with an 
>> explanation of why), and pretty much all current uses of 'instruction' 
>> would use StackFrame::CallAddress instead. 
>>
>> That would be a pretty big change, though. I don't think it's fair to 
>> ask you to do it in this bug. 
>>
>> I'm not fond of putting per-processor knowledge about how to get the 
>> true instruction address, in the printing function. I would rather see 
>> the hair isolated in the architecture-specific code that needs it. 
>>
>> One alternative would be to add a virtual method 'ReturnAddress', to 
>> StackFrame, with a definition in the base class that simply returns 
>> 'instruction'. StackFrameX86, etc. could override that to undo the 
>> offset that their unwinders apply to StackFrame::instruction. And then 
>> PrintStack could call StackFrame::ReturnAddress to get the right thing 
>> on all architectures. 
>>
>>
>>
>> http://breakpad.appspot.com/514002/diff/1/src/processor/minidump_stackwalk.cc

>> File src/processor/minidump_stackwalk.cc (right): 
>>
>>
http://breakpad.appspot.com/514002/diff/1/src/processor/minidump_stackwalk.cc...

>>
>>
src/processor/minidump_stackwalk.cc:148<http://breakpad.appspot.com/514002/diff/1/src/processor/minidump_stackwalk.cc#newcode148src/processor/minidump_stackwalk.cc:148>:

>> // 
>> StackwalkerAMD64::GetCallerFrame) subtracts one from the instruction 
>> pointer 
>> Does the inserted code use tab characters here? It's Google style to use 
>> only space characters. 
>>
>> http://breakpad.appspot.com/514002/ 
>>
>

[jimb]

This looks great! I had some thoughts about the comments, and a suggestion for
the 'if's.

http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/st...
File src/google_breakpad/processor/stack_frame.h (right):

http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/st...
src/google_breakpad/processor/stack_frame.h:90: virtual u_int64_t
ReturnAddress() const { return instruction; }
This is the place for the explanation of why it's useful to distinguish the
return address and the 'instruction'. The comment from minidump_stackwalk.cc,
explaining that 'instruction' is the right thing for looking up information
about the *call*, whereas this is the address at which execution will *resume*
(are we looking backwards in time, like a "backtrace", or forwards in time, like
a "continuation"?) should be here, explaining the method's meaning and reason
for existing.

http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/st...
File src/google_breakpad/processor/stack_frame_cpu.h (right):

http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/st...
src/google_breakpad/processor/stack_frame_cpu.h:84: virtual u_int64_t
ReturnAddress() const;
I don't think it's helpful to repeat the the comment from StackFrame here. Just
a one-line note saying that this is an override should be plenty.

http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/st...
src/google_breakpad/processor/stack_frame_cpu.h:159: // for details.
Similarly: no need to repeat the comment here; just note the override.

http://breakpad.appspot.com/514002/diff/5001/src/processor/minidump_stackwalk.cc
File src/processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/514002/diff/5001/src/processor/minidump_stackwalk...
src/processor/minidump_stackwalk.cc:152: u_int64_t instruction_address =
frame->ReturnAddress();
I don't think any comment is needed here. This code operates on the return
address, and that much is clear from the member function's name alone.

http://breakpad.appspot.com/514002/diff/5001/src/processor/minidump_stackwalk...
src/processor/minidump_stackwalk.cc:290: u_int64_t instruction_address =
frame->ReturnAddress();
Similarly; let the definition of ReturnAddress in StackFrame do its job.

http://breakpad.appspot.com/514002/diff/5001/src/processor/stackwalker_amd64.cc
File src/processor/stackwalker_amd64.cc (right):

http://breakpad.appspot.com/514002/diff/5001/src/processor/stackwalker_amd64....
src/processor/stackwalker_amd64.cc:108: return instruction;
I think what you want here is to assert that RIP is valid, and then just return
it unconditionally.

Remember to put the #include <assert.h> in the right place, as per Google C++
Style.

http://breakpad.appspot.com/514002/diff/5001/src/processor/stackwalker_x86.cc
File src/processor/stackwalker_x86.cc (right):

http://breakpad.appspot.com/514002/diff/5001/src/processor/stackwalker_x86.cc...
src/processor/stackwalker_x86.cc:113: return instruction;
Same here.

[bruce.dawson]

Yep, those all make sense and were inline with my inclinations. I just 
posted an updated patch set.
 
I used the style guide I found at 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml to guide my 
placement of the assert.h include.

On Tuesday, January 22, 2013 8:51:07 AM UTC-8, Jim Blandy wrote:

> This looks great! I had some thoughts about the comments, and a 
> suggestion for the 'if's. 
>
>
>
>
http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/st...

> File src/google_breakpad/processor/stack_frame.h (right): 
>
>
http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/st...

>
>
src/google_breakpad/processor/stack_frame.h:90<http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/stack_frame.h#newcode90src/google_breakpad/processor/stack_frame.h:90>:

> virtual u_int64_t 
> ReturnAddress() const { return instruction; } 
> This is the place for the explanation of why it's useful to distinguish 
> the return address and the 'instruction'. The comment from 
> minidump_stackwalk.cc, explaining that 'instruction' is the right thing 
> for looking up information about the *call*, whereas this is the address 
> at which execution will *resume* (are we looking backwards in time, like 
> a "backtrace", or forwards in time, like a "continuation"?) should be 
> here, explaining the method's meaning and reason for existing. 
>
>
>
http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/st...

> File src/google_breakpad/processor/stack_frame_cpu.h (right): 
>
>
http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/st...

>
>
src/google_breakpad/processor/stack_frame_cpu.h:84<http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/stack_frame_cpu.h#newcode84src/google_breakpad/processor/stack_frame_cpu.h:84>:

> virtual u_int64_t 
> ReturnAddress() const; 
> I don't think it's helpful to repeat the the comment from StackFrame 
> here. Just a one-line note saying that this is an override should be 
> plenty. 
>
>
http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/st...

>
>
src/google_breakpad/processor/stack_frame_cpu.h:159<http://breakpad.appspot.com/514002/diff/5001/src/google_breakpad/processor/stack_frame_cpu.h#newcode159src/google_breakpad/processor/stack_frame_cpu.h:159>:

> // for details. 
> Similarly: no need to repeat the comment here; just note the override. 
>
>
>
http://breakpad.appspot.com/514002/diff/5001/src/processor/minidump_stackwalk.cc

> File src/processor/minidump_stackwalk.cc (right): 
>
>
http://breakpad.appspot.com/514002/diff/5001/src/processor/minidump_stackwalk...

>
>
src/processor/minidump_stackwalk.cc:152<http://breakpad.appspot.com/514002/diff/5001/src/processor/minidump_stackwalk.cc#newcode152src/processor/minidump_stackwalk.cc:152>:

> u_int64_t instruction_address = 
> frame->ReturnAddress(); 
> I don't think any comment is needed here. This code operates on the 
> return address, and that much is clear from the member function's name 
> alone. 
>
>
http://breakpad.appspot.com/514002/diff/5001/src/processor/minidump_stackwalk...

>
>
src/processor/minidump_stackwalk.cc:290<http://breakpad.appspot.com/514002/diff/5001/src/processor/minidump_stackwalk.cc#newcode290src/processor/minidump_stackwalk.cc:290>:

> u_int64_t instruction_address = 
> frame->ReturnAddress(); 
> Similarly; let the definition of ReturnAddress in StackFrame do its job. 
>
>
>
http://breakpad.appspot.com/514002/diff/5001/src/processor/stackwalker_amd64.cc

> File src/processor/stackwalker_amd64.cc (right): 
>
>
http://breakpad.appspot.com/514002/diff/5001/src/processor/stackwalker_amd64....

>
>
src/processor/stackwalker_amd64.cc:108<http://breakpad.appspot.com/514002/diff/5001/src/processor/stackwalker_amd64.cc#newcode108src/processor/stackwalker_amd64.cc:108>:

> return instruction; 
> I think what you want here is to assert that RIP is valid, and then just 
> return it unconditionally. 
>
> Remember to put the #include <assert.h> in the right place, as per 
> Google C++ Style. 
>
>
> http://breakpad.appspot.com/514002/diff/5001/src/processor/stackwalker_x86.cc

> File src/processor/stackwalker_x86.cc (right): 
>
>
http://breakpad.appspot.com/514002/diff/5001/src/processor/stackwalker_x86.cc...

>
>
src/processor/stackwalker_x86.cc:113<http://breakpad.appspot.com/514002/diff/5001/src/processor/stackwalker_x86.cc#newcode113src/processor/stackwalker_x86.cc:113>:

> return instruction; 
> Same here. 
>
> http://breakpad.appspot.com/514002/ 
>

[jimb]

"Looks Good To Me".

http://breakpad.appspot.com/514002/diff/13001/src/google_breakpad/processor/s...
File src/google_breakpad/processor/stack_frame.h (right):

http://breakpad.appspot.com/514002/diff/13001/src/google_breakpad/processor/s...
src/google_breakpad/processor/stack_frame.h:91: virtual u_int64_t
ReturnAddress() const { return instruction; }
This is fine. Following the reference, though, I see that we also need to update
the comments in stackwalker_x86 and stackwalker_amd64 to refer to
StackFrame::ReturnAddress instead of telling people to use the context's
register value.

I'll make that change and land the patch for you. Thanks for the work!

[jimb]

Landed as r1105.

[bruce.dawson]

Thanks Jim.


On Tue, Jan 22, 2013 at 2:38 PM, <jimblandy@gmail.com> wrote:

> Landed as r1105.
>
> http://breakpad.appspot.com/**514002/<http://breakpad.appspot.com/514002/>
>