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

[Jason Merrill <jason at redhat dot com>]

Ye, Joey wrote:

Your patch suffered from word wrap.

> +  /* Whether stack realign uses Dynamic Realign Argument Pointer.  */
> +  unsigned is_drap : 1;

I'd prefer uses_drap.

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

> +	      /* 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.

Also, all these rules seem to assume that the DRAP register will be FP.

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

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

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

> +      /* 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.

Jason