Bug in os::current_frame()/_get_previous_fp() or why parenthesis somtimes really matter

[steve goldman]

Volker Simonis wrote:
> Hi,
> 
> the implementation of "_get_previous_fp()" which is used by
> "os::current_frame()" seems to be buggy on Solaris/x86_64.
> "_get_previous_fp()" is implemented as assembler inline function in
> "hotspot/src/os_cpu/solaris_amd64/vm/solaris_amd64.il" as follows:
> 
>   // Get the frame pointer from previous frame.
>       .inline _get_previous_fp,0
>       movq     %rbp, %rax
>       movq     %rax, %rax
>       .end

This seems to be correct. Since no new frame pointer  is created,  the 
value of rbp is the "fp" of the previous frame, not the previous value 
of rbp. Certainly the definition of previous is ambiguous here but the 
.il file on the 32bit side does say it returns the caller's fp. The 
comment in the 64bit version is more ambiguous.

> 
> Although I'm not an x86 expert I think that after locking at
> "generate_get_previous_fp()" in
> hotspot/src/cpu/amd64/vm/stubGenerator_amd64.cpp which generated a
> stub that was used previously in  "os::current_frame()" to query the
> frame pointer and looks as follows:
> 
> address generate_get_previous_fp() {
>   StubCodeMark mark(this, "StubRoutines", "get_previous_fp");
>   const Address old_fp(rbp, 0);
>   const Address older_fp(rax, 0);
>   address start = __ pc();
> 
>   __ enter();
>   __ movq(rax, old_fp); // callers fp
>   __ movq(rax, older_fp); // the frame for ps()
>   __ popq(rbp);
>   __ ret(0);
> 

this code is incorrect. The frame pointer that is desired in 
os::current_frame is the frame pointer that exists for itself. The frame 
it constructs uses "previous fp" (it's own fp) and current sp. That is a 
sensible frame.

The incorrect code would create a frame whose fp was from the call of 
os::current_frame and whose sp was the sp for os::current_frame which is 
not a proper description of the frame.

For what this particular frame is used for it is completely harmless 
since we won't be doing anything with sp. The truth is almost any value 
of fp that is returned that is actually a frame pointer is usable since 
the stack walking the caller's are going to do is dependent on fp being 
a valid frame pointer and that's about it.

>   return start;
> }
> 
> the correct implementation of  "_get_previous_fp()" should read as follows:
> 
>   // Get the frame pointer from previous frame.
>       .inline _get_previous_fp,0
>       movq     (%rbp), %rax
>       movq     (%rax), %rax
>       .end
> 
> The funny thing is that although the current implementation is clearly
> wrong, it still worked in the debug build. I think this has to do with
> how CC treats inline assembly under optimization. In the debug build,
> it generated the following code:
> 
>            185. frame os::current_frame() {
>                 <Function: os::current_frame()>
>                 [ 185]      780:  pushq   %rbp
>                 [ 185]      781:  movq    %rsp,%rbp
>                 [ 185]      784:  subq    $0xc0,%rsp
>                 [ 185]      78b:  movq    %r12,-0xb8(%rbp)
>                 [ 185]      792:  movq    %rdi,-8(%rbp)
>            186.   intptr_t* fp = _get_previous_fp();
>                 [ 186]      796:  movl    $0,%eax
>                 [ 186]      79b:  movq    %rbp,%rax
>                 [ 186]      79e:  movq    %rax,%rax
>                 [ 186]      7a1:  movq    %rax,%r8
>                 [ 186]      7a4:  movq    %r8,-0x40(%rbp)
>            187.   frame myframe((intptr_t*)os::current_stack_pointer(),
> 
> This code returned the wrong, but still a valid fp which is just good
> enough for "os::current_frame()". However if compiled with '-xO1' as
> done in the opt build, CC generates the following code:
> 
>                 <Function: os::current_frame()>
>                 [  ?]        0:  pushq   %rbp
>                 [  ?]        1:  movq    %rsp,%rbp
>                 [  ?]        4:  subq    $0x60,%rsp
>                 [  ?]        8:  pushq   %r12
>                 [  ?]        a:  pushq   %r13
>                 [  ?]        c:  movq    %rdi,%r12
>                 [  ?]        f:  movq    %rax,%r13
>                 [  ?]       12:  call    os::current_stack_pointer() [
> 0x17, .+5 ]
> 
> You can see that it elliminated the code from "_get_previous_fp()" and
> just used the uninitialized value of %rax as frame pointer, which is
> usually wrong.

I think this is a CC bug. How is it able to eliminate the read of rbp 
and simply give us a random value in rax? Maybe the .il file needs some 
annotation so that the compiler doesn't decide that rbp is 
"un-initialized" and so any old value in rax is as good as another.


> 
> Notice that this problem exists in the latest JDK 6 build as well as
> in OpenJDK (Java 7). It probably hasn't showed up until now because
> "os::current_frame()" is only called in the following three places:
> 
> 1.oop::register_oop() in /hotspot/src/share/vm/oops/oopsHierarchy.cpp
> 2.ps() in hotspot/src/share/vm/utilities/debug.cpp
> 3. VMError::report(outputStream* st) in
> /hotspot/src/share/vm/utilities/vmError.cpp
> 
> The first one is usually disabled, the second one is only called by
> developers in the debugger (and probably for a long time nobody called
> "ps()" in the debugger for an opt-build) and the last one is during
> the creation of an hs_error file where probably nobody noticed the
> problem.
> 
> Regards,
> Volker


-- 
Steve