Jakub Jelinek <ja...@redhat.com> writes:
> On Tue, Feb 04, 2014 at 12:21:14PM +0000, Richard Sandiford wrote:
>> Jakub Jelinek <ja...@redhat.com> writes:
>> >> But then we wouldn't be able to use var-tracking when __builtin_eh_return
>> >> is used, since in that case replacing the (set (reg ...) (mem ...))
>> >> with a (plus ...) would be incorrect -- the value we're loading from the
>> >> stack will have had a variable adjustment applied. And I know from
>> >> painful
>> >> experience that being able to debug the unwind code is very useful. :-)
>> >
>> > Aren't functions using EH_RETURN typically using frame pointer?
>>
>> Sorry, forgot to respond to this bit. On s390, _Unwind_RaiseException and
>> _Unwind_ForcedUnwind don't use a frame pointer, at least not on trunk.
>> %r11 is used as a general scratch register instead. E.g.:
>>
>> 00002ba8 <_Unwind_ForcedUnwind>:
>> 2ba8: 90 6f f0 18 stm %r6,%r15,24(%r15)
>> 2bac: 0d d0 basr %r13,%r0
>> 2bae: 60 40 f0 50 std %f4,80(%r15)
>> 2bb2: 60 60 f0 58 std %f6,88(%r15)
>> 2bb6: a7 fa fd f8 ahi %r15,-520
>> 2bba: 58 c0 d0 9e l %r12,158(%r13)
>> 2bbe: 58 10 d0 9a l %r1,154(%r13)
>> 2bc2: 18 b2 lr %r11,%r2
>> ...
>> 2c10: 98 6f f2 20 lm %r6,%r15,544(%r15)
>> 2c14: 07 f4 br %r4
>
> Looking at pr54693-2.c -O2 -g -m64 -march=z196 (was that the one you meant)
> with your patches, I see:
> (insn/f:TI 122 30 31 4 (parallel [
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 80 [0x50])) [3 S8 A8])
> (reg:DI 10 %r10))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 88 [0x58])) [3 S8 A8])
> (reg:DI 11 %r11))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 96 [0x60])) [3 S8 A8])
> (reg:DI 12 %r12))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 104 [0x68])) [3 S8 A8])
> (reg:DI 13 %r13))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 112 [0x70])) [3 S8 A8])
> (reg:DI 14 %r14))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 120 [0x78])) [3 S8 A8])
> (reg/f:DI 15 %r15))
> ]) pr54693-2.c:16 94 {*store_multiple_di}
> (expr_list:REG_DEAD (reg:DI 14 %r14)
> (expr_list:REG_DEAD (reg:DI 13 %r13)
> (expr_list:REG_DEAD (reg:DI 12 %r12)
> (expr_list:REG_DEAD (reg:DI 11 %r11)
> (expr_list:REG_DEAD (reg:DI 10 %r10)
> (nil)))))))
> ...
> (insn/f 123 31 124 4 (set (reg/f:DI 15 %r15)
> (plus:DI (reg/f:DI 15 %r15)
> (const_int -160 [0xffffffffffffff60]))) pr54693-2.c:16 65 {*la_64}
> (expr_list:REG_FRAME_RELATED_EXPR (set (reg/f:DI 15 %r15)
> (plus:DI (reg/f:DI 15 %r15)
> (const_int -160 [0xffffffffffffff60])))
> (nil)))
> ...
> (insn/f:TI 135 134 136 7 (parallel [
> (set (reg:DI 10 %r10)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 240 [0xf0])) [3 S8 A8]))
> (set (reg:DI 11 %r11)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 248 [0xf8])) [3 S8 A8]))
> (set (reg:DI 12 %r12)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 256 [0x100])) [3 S8 A8]))
> (set (reg:DI 13 %r13)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 264 [0x108])) [3 S8 A8]))
> (set (reg:DI 14 %r14)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 272 [0x110])) [3 S8 A8]))
> (set (reg/f:DI 15 %r15)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 280 [0x118])) [3 S8 A8]))
> ]) pr54693-2.c:25 92 {*load_multiple_di}
> (expr_list:REG_CFA_DEF_CFA (plus:DI (reg/f:DI 15 %r15)
> (const_int 160 [0xa0]))
> (expr_list:REG_CFA_RESTORE (reg/f:DI 15 %r15)
> (expr_list:REG_CFA_RESTORE (reg:DI 14 %r14)
> (expr_list:REG_CFA_RESTORE (reg:DI 13 %r13)
> (expr_list:REG_CFA_RESTORE (reg:DI 12 %r12)
> (expr_list:REG_CFA_RESTORE (reg:DI 11 %r11)
> (expr_list:REG_CFA_RESTORE (reg:DI 10 %r10)
> (nil)))))))))
> In a function that doesn't need frame pointer, I'd say this is a serious
> bloat of the unwind info, you are saving/restoring %r15 not because you have
> to, but just that it seems to be cheaper for the target. So, I'd say you
> shouldn't emit DW_CFA_offset 15, 5 on the insn 122 in the prologue and
> DW_CFA_restore 15 in the epilogue (twice in this case), that just waste
> of .eh_frame/.debug_frame space. I'd say you should represent in
> this case the *store_multiple_di as REG_FRAME_RELATED_EXPR
> with all but the last set in the PARALLEL, and on the *load_multiple_di
> remove REG_CFA_RESTORE (r15) note and change REG_CFA_DEF_CFA note to
> REG_CFA_ADJUST_CFA note which would say that stack pointer has been
> incremented by 160 bytes (epilogue expansion knows that value).
>
> Then just handle REG_CFA_ADJUST_CFA note in
> insn_stack_adjust_offset_pre_post. Handling REG_CFA_DEF_CFA
> note there would be harder (as you need to know the current cfa at that
> point), so perhaps just punt there if you see REG_CFA_DEF_CFA note.
What do you want var-tracking to do about the __builtin_eh_return case
though? It isn't correct to say that the instruction adds the frame size
to the stack pointer then, since the loaded value includes the eh_return
adjustment. Pretending it is a constant addition in order to satisfy the
sanity check seems a bit hackish, but there again it'd be a shame to lose
tracking for the whole function just because of that.
Thanks,
Richard
