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

[Volker Simonis]

On 2/8/08, steve goldman <Steve.Goldman at sun.com> wrote:
> 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.
>

Ok, I agree with you on this. But what is the line:

movq     %rax, %rax

good for. Isn't it just a nop?

> >
> > 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.
>

I also agree on this one - seems to be a compiler bug. I found Alfred
Huang's weblog at
http://blogs.sun.com/alblog/entry/on_studio_and_gcc_style  where he
explains that inline assembler templates get heavily optimzed by the
backend ('ube' on x86). The solution is to use the compiler option
'-Wu,-no_a2lf' for C or "-Qoption ube -no_a2lf" for C++ to disable
inline assembly optimization.

This can be easily added to
'hotspot/build/solaris/makefiles/amd64.make' which already contains
special opt-flags for 'os_solaris_amd64.o' anyway:

OPT_CFLAGS/os_solaris_amd64.o = -xO1 -Qoption ube -no_a2lf

> >
> > 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
>