Re: [PATCH 3/7]: Ping3: Merge from Stack Branch - DWARF2

["H.J. Lu" <hjl dot tools at gmail dot com>]

Hi,

I am checking this patch into stack branch.

Thanks.


H.J.
---
2008-06-02  Xuepeng Guo  <xuepeng.guo@intel.com>

	* dwarf2out.c (dw_fde_struct): Rename is_drap to uses_drap.
	(add_cfi): Replace is_drap with uses_drap.
	(dwarf2out_frame_debug_expr): Update the comments, replace is_drap
	with uses_drap and add two assertions.
	(reg_save_with_expression): Replace is_drap with uses_drap.

2008/6/1 Guo, Xuepeng <xuepeng.guo@intel.com>:
> Hello Jason,
> Thanks very much for your review. Because this dwarf2 patch is strongly related to other parts of stack realignment, so I will explain some code through examples. Here are my comments, please review them.
> Thanks.
> Xuepeng Guo
>
>>-----Original Message-----
>>From: Jason Merrill [mailto:jason@redhat.com]
>>Sent: 2008年5月31日 0:12
>>To: Ye, Joey
>>Cc: gcc-patches@gcc.gnu.org; Richard Guenther; Lu, Hongjiu; Guo, Xuepeng;
>>Jan Hubicka
>>Subject: Re: [PATCH 3/7]: Ping3: Merge from Stack Branch - DWARF2
>>
>>Ye, Joey wrote:
>>
>>Your patch suffered from word wrap.
>
> I will re-format the patch.
>
>>
>>> +  /* Whether stack realign uses Dynamic Realign Argument Pointer.  */
>>> +  unsigned is_drap : 1;
>>
>>I'd prefer uses_drap.
>
> OK. I will update it.
>
>>
>>> +  Rule 16:
>>> +  (set sp (and: sp <const_int>))
>>> +  effects: current_fde.stack_realign = 1
>>> +           cfa_store.offset = 0
>>
>>Seems like this could use an assert to make sure dest==cfa_store.reg.
>
> OK. I will add an assertion here.
>
>>
>>> +          /* Rule 19 */
>>> +          /* Each time when setting FP to SP under the condition of
>>> +             that the stack is realigned we assume the realign used
>>> +             Dynamic Realign Argument Pointer and the register used
>>> +             is the current cfa's register. We update cfa's register
>>> +             to FP.  */
>>
>>It also seems worth testing that src==cfa.reg rather than assuming it.
>>
> OK. I will add this assertion. Here I maybe misused the word "assume". I just want to say when using drap in stack realignment we will redefine cfa here. There is no supposing here.
>
>>Also, all these rules seem to assume that the DRAP register will be FP.
>>
> The DRAP register is different from FP. They are used for different purpose in our stack realign proposal. Let me explain them through a piece of code in prologue:
>  8048620 <_Z3foov>:
>  8048620:       8d 4c 24 04             lea    0x4(%esp),%ecx
>  8048624:       83 e4 e0                and    $0xffffffe0,%esp
>  8048627:       ff 71 fc                pushl  0xfffffffc(%ecx)
>  804862a:       55                      push   %ebp
>  804862b:       89 e5                   mov    %esp,%ebp
>  804862d:       83 ec 48                sub    $0x48,%esp
> The DRAP register is ecx. Rule 19 is corresponding to asm statement 804862b. We don't assume the DRAP register is FP, we just update the cfa.register to FP here.
>
>>> @@ -3558,6 +3670,9 @@ output_cfa_loc (dw_cfi_ref cfi)
>>>    dw_loc_descr_ref loc;
>>>    unsigned long size;
>>>
>>> +  if (cfi->dw_cfi_opc == DW_CFA_expression)
>>> +    dw2_asm_output_data (1, cfi->dw_cfi_oprnd2.dw_cfi_reg_num, NULL);
>>
>>This seems to violate the DWARF spec.  It looks like you're writing out
>>the initial register number before emitting the location expression,
>>which will confuse any debugger.
>>
> To generation dwarf expression like below:
> DW_CFA_expression: r1 (DW_OP_const4s: 24; DW_OP_minus)
> I have to use this if statement to output the register number of r1 firstly.
> The debugger like gdb does accept it and work well. Without this register number gdb will wrongly decoded this dwarf expression info.
>
>>> -      gcc_assert (elim == (frame_pointer_needed ? hard_frame_pointer_rtx
>>> -                  : stack_pointer_rtx));
>>> +      gcc_assert (MAX_STACK_ALIGNMENT
>>> +                  || elim == (frame_pointer_needed ?
>>hard_frame_pointer_rtx
>>> +                                                   : stack_pointer_rtx));
>>
>>I'd expect to be able to do better than this.  You're saying that if the
>>target supports DRAP, we can eliminate to any register?  This seems at
>>odds with the earlier code that assumes that the DRAP register will
>>always be FP.
>>
> Joey, please add your comments here.
>
>>> +  if (fde->stack_realign)
>>> +    {
>>> +    }
>>> +
>>> +  /* We need to restore register used by Dynamic Realign Argument
>>> +     Pointer through dereference.   */
>>> +  if (fde->is_drap && reg == fde->drap_regnum)
>>> +    {
>>
>>Surely these will be true in the same functions, but it seems that the
>>code in the second if block just throws away the location expression
>>built up in the first one.  It should be possible to reuse it rather
>>than build the same thing up again.
>>
> These two if statements are exclusive. One function can only fall into one of these two if statements. They are designed to generate different dwarf2 expression information for different stack realignment ways. The first if statement is for general stack realignment like:
> 08048620 <_Z3foov>:
>  8048620:       55                      push   %ebp
>  8048621:       89 e5                   mov    %esp,%ebp
>  8048623:       83 e4 f0                and    $0xfffffff0,%esp
>  8048626:       83 ec 20                sub    $0x20,%esp
>  8048629:       89 5c 24 14             mov    %ebx,0x14(%esp)
>  804862d:       89 74 24 18             mov    %esi,0x18(%esp)
>  8048631:       89 7c 24 1c             mov    %edi,0x1c(%esp)
> The second if statement is for stack realignment with DRAP register used like:
> 08048620 <_Z3foov>:
>  8048620:       8d 4c 24 04             lea    0x4(%esp),%ecx
>  8048624:       83 e4 e0                and    $0xffffffe0,%esp
>  8048627:       ff 71 fc                pushl  0xfffffffc(%ecx)
>  804862a:       55                      push   %ebp
>  804862b:       89 e5                   mov    %esp,%ebp
>  804862d:       83 ec 48                sub    $0x48,%esp
>  8048630:       c7 45 c8 01 00 00 00    movl   $0x1,0xffffffc8(%ebp)
>  8048637:       8b 45 c8                mov    0xffffffc8(%ebp),%eax
>  804863a:       89 4c 24 38             mov    %ecx,0x38(%esp)
>  804863e:       89 5c 24 3c             mov    %ebx,0x3c(%esp)
>  8048642:       89 74 24 40             mov    %esi,0x40(%esp)
> So here I can not reuse them.
>
>>> +      /* We also need to properly restore the caller's stack pointer.
>>> +     But if callee calls __builtin_eh_return, the calculation of
>>> +     offset from exception handling function (typically is
>>> +     _Unwind_RaiseException) to target function during unwinding
>>> +     will conflict with this mechanism.  It will simply set offset
>>> +     to zero.  So here if functions call __builtin_eh_return,
>>> +     there will be no unwind information for restoring the stack
>>> +     pointer.  */
>>
>>So this will break unwinding in the debugger out of
>>_Unwind_RaiseException?  I'm not excited about that.  What would it take
>>to get accurate unwind info?
>>
>>Also, you're allocating cfi2 unconditionally but only using it in this
>>if block.
>>
>>It seems like it would be simpler to just disable DRAP when
>>calls_eh_return if generating accurate unwind information is too
>>complicated.
>>
> In stack realignment with DRAP register used, we will explicitly generate dwarf expression information (cfi2) to tell the debugger how to properly restore the stack pointer (r4 in dwarf2). But this is not suitable for _Unwind_RaiseException function because it calls __builtin_eh_return. So here we just don't generate cfi2 for _Unwind_RaiseException function and this works well. We can not disable DRAP for _Unwind_RaiseException function. This function needs DRAP. BTW: _Unwind_RaiseException is the only function that calls __builtin_eh_return.
>
>>Jason
>



-- 
H.J.

Attachment: d.txt
Description: Text document