Incorrect unwind data in entry.S

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

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

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.

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

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

Hi Cary,

Thanks for your feedback. Here some comments:

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

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

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:

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

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