[Linux-ia64] Incorrect unwind data in entry.S

[Linux-ia64] Incorrect unwind data in entry.S

[Keith Owens]

Either I have completely misunderstood the IA64 unwind data (quite
likely) or this code is wrong.

GLOBAL_ENTRY(ia64_prepare_handle_unaligned)
        //
        // r16 = fake ar.pfs, we simply need to make sure
        // privilege is still 0
        //
        PT_REGS_UNWIND_INFO(0)
        mov r16=r0
        UNW(.prologue)
        DO_SAVE_SWITCH_STACK
        br.call.sptk.few rp=ia64_handle_unaligned // stack frame setup in ivt
.ret21: .body
        DO_LOAD_SWITCH_STACK(PT_REGS_UNWIND_INFO(0))
        br.cond.sptk.many rp                      // goes to ia64_leave_kernel
END(ia64_prepare_handle_unaligned)

That preprocesses into

.global ia64_prepare_handle_unaligned; .align 32; .proc ia64_prepare_handle_unaligned; ia64_prepare_handle_unaligned:
        .prologue;			// prologue 1
        .unwabi @svr4, 'i';
        .fframe 400+16+(0);
        // assorted .spillsp directives
        .body
        mov r16=r0
        .prologue			// prologue 2
        movl r28\x1f;
        ;;
        .fframe 576;
        adds sp=-576,sp;
        mov b7=r28;
        // assorted .savesp and .spillsp directives
        br.cond.sptk.many save_switch_stack;
        1:
        br.call.sptk.few rp=ia64_handle_unaligned
.ret21: .body
        movl r28\x1f;
        ;;
        mov b7=r28;
        br.cond.sptk.many load_switch_stack;
        1: .restore sp;			// pop prologue 2
        .prologue;			// prologue 3
        .unwabi @svr4, 'i';
        .fframe 400+16+(0);
        .spillsp rp, (8 + 16)+(0);
        .spillsp ar.pfs, (16 + 16)+(0);
        .spillsp ar.unat, (24 + 16)+(0);
        .spillsp ar.fpsr, (312 + 16)+(0);
        .spillsp pr, (64 + 16)+(0);;
        .body;
        adds spW6,sp
        br.cond.sptk.many rp
.endp ia64_prepare_handle_unaligned

The prologue after .ret21 makes no sense.  Unwind claims that we are
increasing the stack by 416 and spilling registers to stack but we are
really removing the struct switch_stack.

The mismatch between unwind and the code does not cause any problems
because we hit .endp almost immediately.  I just want to get
confirmation that this really is an error.  AFAICT, prologue 3 should
be an epilog.

Re: [Linux-ia64] Incorrect unwind data in entry.S

[David Mosberger]

>>>>> On Thu, 21 Dec 2000 14:42:45 +1100, Keith Owens <kaos@ocs.com.au> said:

  Keith> The prologue after .ret21 makes no sense.  Unwind claims that
  Keith> we are increasing the stack by 416 and spilling registers to
  Keith> stack but we are really removing the struct switch_stack.

The code is correct.  A prologue always describes the _current state_
of the frame, not the _changes_ to the frame (which would make no
sense).  In other words, the code says that after the
load_switch_stack, the frame is back to the original state (switch
stack is gone).

	--david

Re: [Linux-ia64] Incorrect unwind data in entry.S

[Keith Owens]

On Fri, 5 Jan 2001 10:48:45 -0800, 
David Mosberger <davidm@hpl.hp.com> wrote:
>>>>>> On Thu, 21 Dec 2000 14:42:45 +1100, Keith Owens <kaos@ocs.com.au> said:
>
>  Keith> The prologue after .ret21 makes no sense.  Unwind claims that
>  Keith> we are increasing the stack by 416 and spilling registers to
>  Keith> stack but we are really removing the struct switch_stack.
>
>The code is correct.  A prologue always describes the _current state_
>of the frame, not the _changes_ to the frame (which would make no
>sense).  In other words, the code says that after the
>load_switch_stack, the frame is back to the original state (switch
>stack is gone).

Where does it say that a prologue always describes the current state?
I am looking at Intel's IA-64 Software Conventions and Runtime
Architecture Guide, 24535802.pdf, August 2000.  Section 11.3

  "For the purposes of unwinding, we divide every procedure up into one
  or more regions, which are classified as either "prologue" or "body"
  regions."

  ...

  "For both types of regions, the unwinder needs to know the state of
  the stack frames and preserved registers upon entry to the region.
  There are four ways to establish the entry state for an unwind
  region:

  * The first region in the procedure assumes that both stack frames
    are unallocated, and no registers have been saved upon entry to the
    region.
  * A region may modify the state of the stack frames and preserved
    registers; each subsequent region takes the previous region's exit
    state as its entry state.
  * When control does not flow into a region from directly above it,
    the region may specify an alternate predecessor region whose exit
    state is used instead.
  * Zero-length prologue regions may be inserted just prior to a
    prologue or body region to set up the correct entry state."

The first region in a procedure has no initial state, so the first
prologue must describe the current state, we agree on that.  The second
point explicitly says that "each subsequent region takes the previous
region's exit state as its entry state", this point makes no
distinction between body and prologue regions.

According to the docs, a second prologue in a procedure should only
modify the existing state, not define the entire state.  Either the
documentation is incorrect or the unwind data for the second prologue
in ia64_prepare_handle_unaligned is incorrect.

Re: [Linux-ia64] Incorrect unwind data in entry.S

[David Mosberger]

>>>>> On Sat, 06 Jan 2001 11:51:17 +1100, Keith Owens <kaos@ocs.com.au> said:

  Keith> Where does it say that a prologue always describes the
  Keith> current state?

The fframe directive describes the "size of the frame".  It's not a
delta.

	--david

Re: [Linux-ia64] Incorrect unwind data in entry.S

[Cary Coutant]

I apologize for taking so long to respond to this, but I've been trying 
to convince myself that there is a defect in the assembly language 
specification. Specifically, there seems to be no directive to specify 
that a body region has any epilogue code, or what the epilogue count 
should be.

In the example code quoted below, it's clear to me that the first body 
region (following prologue 1) should have no epilogue descriptor, but 
that the second body region (following prologue 2, at label .ret21) 
should. It's not, however, clear, whether the epilogue count should be 0 
(meaning that the epilogue balances only prologue 2) or 1 (meaning that 
it balances both prologues). The intent of this code seems to be that the 
epilogue count should be 0, since the comment says "pop prologue 2", and 
execution continues with a body region that logically matches the state 
of the first body region.

There are a couple of problems here, though.

First, assuming that the epilogue count for body region 2 is 0, as 
intended, the empty prologue 3 region shouldn't be at all necessary, 
since its only purpose is to reconstruct the unwind state that existed 
prior to prologue 2. With the proper epilogue count in body region 2, the 
unwind state for body region 3 should follow naturally.

Second, nested prologues weren't designed to handle a variable frame 
size. The unwinder assumes that a fixed stack frame size applies 
throughout the procedure. If you're going to change the size of your 
frame, you should copy the previous stack pointer (psp) into a local 
register, and declare a variable-size frame with the .vframe directive. 
You can then change the value of sp whenever you like, and the unwinder 
will always know to look in that local register for the value of psp.

If you use .vframe and save psp in a local register, you won't have any 
need for nested prologue regions here.

Nevertheless, I think we need to add a .epilog directive to the assembler 
before someone really needs it.

-cary


>        .prologue;			// prologue 1
>        .unwabi @svr4, 'i';
>        .fframe 400+16+(0);
>        // assorted .spillsp directives
>        .body
>        mov r16=r0
>        .prologue			// prologue 2
>        movl r28\x1f;
>        ;;
>        .fframe 576;
>        adds sp=-576,sp;
>        mov b7=r28;
>        // assorted .savesp and .spillsp directives
>        br.cond.sptk.many save_switch_stack;
>        1:
>        br.call.sptk.few rp=ia64_handle_unaligned
>.ret21: .body
>        movl r28\x1f;
>        ;;
>        mov b7=r28;
>        br.cond.sptk.many load_switch_stack;
>        1: .restore sp;			// pop prologue 2
>        .prologue;			// prologue 3
>        .unwabi @svr4, 'i';
>        .fframe 400+16+(0);
>        .spillsp rp, (8 + 16)+(0);
>        .spillsp ar.pfs, (16 + 16)+(0);
>        .spillsp ar.unat, (24 + 16)+(0);
>        .spillsp ar.fpsr, (312 + 16)+(0);
>        .spillsp pr, (64 + 16)+(0);;
>        .body;
>        adds spW6,sp
>        br.cond.sptk.many rp
>.endp ia64_prepare_handle_unaligned
>
>The prologue after .ret21 makes no sense.  Unwind claims that we are
>increasing the stack by 416 and spilling registers to stack but we are
>really removing the struct switch_stack.
>
>The mismatch between unwind and the code does not cause any problems
>because we hit .endp almost immediately.  I just want to get
>confirmation that this really is an error.  AFAICT, prologue 3 should
>be an epilog.

Re: [Linux-ia64] Incorrect unwind data in entry.S

[David Mosberger]

Hi Cary,

Thanks for your feedback.  Here some comments:

>>>>> On Tue, 16 Jan 2001 15:41:25 -0800, Cary Coutant <cary@cup.hp.com> said:

  Cary> I apologize for taking so long to respond to this, but I've
  Cary> been trying to convince myself that there is a defect in the
  Cary> assembly language specification. Specifically, there seems to
  Cary> be no directive to specify that a body region has any epilogue
  Cary> code, or what the epilogue count should be.

There is, it just has a strange name: ".restore sp" actually generates
an epilogue directive and the (optional) second argument specifies the
epilogue count.

  Cary> In the example code quoted below, it's clear to me that the
  Cary> first body region (following prologue 1) should have no
  Cary> epilogue descriptor, but that the second body region
  Cary> (following prologue 2, at label .ret21) should.

Yes, and does indeed due to the ".restore sp" at label "1:".

  Cary> It's not, however, clear, whether the epilogue count should be
  Cary> 0 (meaning that the epilogue balances only prologue 2) or 1
  Cary> (meaning that it balances both prologues). The intent of this
  Cary> code seems to be that the epilogue count should be 0, since
  Cary> the comment says "pop prologue 2", and execution continues
  Cary> with a body region that logically matches the state of the
  Cary> first body region.

  Cary> There are a couple of problems here, though.

  Cary> First, assuming that the epilogue count for body region 2 is
  Cary> 0, as intended, the empty prologue 3 region shouldn't be at
  Cary> all necessary, since its only purpose is to reconstruct the
  Cary> unwind state that existed prior to prologue 2.  With the
  Cary> proper epilogue count in body region 2, the unwind state for
  Cary> body region 3 should follow naturally.

Ah, that's true.  That should result in a more optimal encoding.

  Cary> Second, nested prologues weren't designed to handle a variable
  Cary> frame size. The unwinder assumes that a fixed stack frame size
  Cary> applies throughout the procedure. If you're going to change
  Cary> the size of your frame, you should copy the previous stack
  Cary> pointer (psp) into a local register, and declare a
  Cary> variable-size frame with the .vframe directive.  You can then
  Cary> change the value of sp whenever you like, and the unwinder
  Cary> will always know to look in that local register for the value
  Cary> of psp.

The kernel unwinder does _not_ assume that the stack size is fixed
throughout the entire procedure.  I didn't think the unwind specs
forces that either, but if I'm missing something please let me know.

  Cary> If you use .vframe and save psp in a local register, you won't
  Cary> have any need for nested prologue regions here.

Since this is kernel exit code, performance is critical and we don't
want to move things around just because of the unwind info.  Also, it
could be difficult to find a register that would be available in all
places that these macros are used.

Thanks,

	--david

Re: [Linux-ia64] Incorrect unwind data in entry.S

[Cary Coutant]

>There is, it just has a strange name: ".restore sp" actually generates
>an epilogue directive and the (optional) second argument specifies the
>epilogue count.

Yes, it looks like Intel added it to their assembler, and I was unaware 
of it until now.

>The kernel unwinder does _not_ assume that the stack size is fixed
>throughout the entire procedure.  I didn't think the unwind specs
>forces that either, but if I'm missing something please let me know.

On reflection, I agree. I didn't anticipate nested prologues modifying a 
fixed frame size, but I did not rule that out in the specs.

There still seems to be a problem with the unwind data not matching the 
actual frame state:

>        .prologue;			// prologue 1
>        .unwabi @svr4, 'i';
>        .fframe 400+16+(0);
>        // assorted .spillsp directives
>        .body
>        mov r16=r0
>        .prologue			// prologue 2
>        movl r28\x1f;
>        ;;
>        .fframe 576;
>        adds sp=-576,sp;
>        mov b7=r28;
>        // assorted .savesp and .spillsp directives
>        br.cond.sptk.many save_switch_stack;
>        1:
>        br.call.sptk.few rp=ia64_handle_unaligned
>.ret21: .body
>        movl r28\x1f;
>        ;;
>        mov b7=r28;
>        br.cond.sptk.many load_switch_stack;
>        1: .restore sp;			// pop prologue 2
>        .prologue;			// prologue 3
>        .unwabi @svr4, 'i';
>        .fframe 400+16+(0);
>        .spillsp rp, (8 + 16)+(0);
>        .spillsp ar.pfs, (16 + 16)+(0);
>        .spillsp ar.unat, (24 + 16)+(0);
>        .spillsp ar.fpsr, (312 + 16)+(0);
>        .spillsp pr, (64 + 16)+(0);;
>        .body;
>        adds spW6,sp
>        br.cond.sptk.many rp
>.endp ia64_prepare_handle_unaligned

Perhaps there's something in the first set of "assorted .spillsp 
directives," but I don't see where the outer 416-byte stack frame is 
being allocated or deallocated. The code then allocates an additional 576 
bytes, but prologue 2 describes the *total* frame size as 576. It then 
pops prologue 2, but doesn't deallocate the 576 bytes until the beginning 
of body region 3, leaving a narrow window where the unwinder will think 
the frame size is 416 bytes.

There must be something I'm missing here.

-cary

Re: [Linux-ia64] Incorrect unwind data in entry.S

[David Mosberger]

>>>>> On Wed, 17 Jan 2001 10:54:26 -0800, Cary Coutant <cary@cup.hp.com> said:

  Cary> Perhaps there's something in the first set of "assorted
  Cary> .spillsp directives," but I don't see where the outer 416-byte
  Cary> stack frame is being allocated or deallocated.

That happens in a very different place of the kernel (the IVT).
Basically, what's happening is that a code fragment A sets up a stack
frame and then calls a handler B, while arranging that B will return
to a code fragment C.  Logically, A and C form a single procedure, but
there really are several different As (and only one C).

  Cary> The code then allocates an additional 576 bytes, but prologue
  Cary> 2 describes the *total* frame size as 576.  It then pops
  Cary> prologue 2, but doesn't deallocate the 576 bytes until the
  Cary> beginning of body region 3, leaving a narrow window where the
  Cary> unwinder will think the frame size is 416 bytes.

Oops, you are right: the size/offsets should have been adjusted by the
size of the first frame.  However, I think we can just drop prologue 1
and 3.  I believe the reason we used to need those was because the
return pointer didn't point to the right place (because the handler B
knew that it's going to "return" to C and simply hardcoded a branch to
C), but with the current setup, "rp" (or the location at which it was
saved) will point to the right place so there is no need to manually
craft unwind info for the frame set up by A.

	--david