Re: [PATCH] dwarf: Multi-register CFI address support.

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Nov 11, 2021 at 06:12:50PM +0000, Hafiz Abid Qadeer wrote:
> 	* dwarf2cfi.c (dw_stack_pointer_regnum): Change type to struct cfa_reg.
> 	(dw_frame_pointer_regnum): Likewise.
> 	(new_cfi_row): Use set_by_dwreg.
> 	(get_cfa_from_loc_descr): Use set_by_dwreg.  Support register spans.
> 	handle DW_OP_bregx with DW_OP_breg{0-31}. Support DW_OP_lit*,

2 spaces instead of 1 before Support above.

> +	      cfa->reg.set_by_dwreg ((op == DW_OP_bregx)
> +		? (ptr->dw_loc_oprnd1.v.val_int) : (op - DW_OP_breg0));

Formatting.  All those 3 () inner pairs are unnecessary, and it would be
nicer to use a temporary, like:
	      unsigned regno
		= (op == DW_OP_bregx
		   ? ptr->dw_loc_oprnd1.v.val_int : op - DW_OP_breg0);
	      cfa->reg.set_by_dwreg (regno);

> +	      unsigned int regno = (op == DW_OP_bregx)
> +		? (ptr->dw_loc_oprnd1.v.val_int) : (op - DW_OP_breg0);

With the ()s similarly, and also ? should be below op.

> +	      gcc_assert (regno == (cfa->reg.reg - 1));

Again, the inner () pair is unnecessary.
> +	      cfa->reg.span_width = (cfa->offset.to_constant () / 8);

And here the outer () pair.

> +
> +#if CHECKING_P

Please use if (CHECKING_P) instead of #if.

> +      /* Ensure that the above assumption is accurate.  */
> +      for (unsigned int i = 0; i < result.span; i++)
> +	{
> +	  gcc_assert (known_eq (GET_MODE_SIZE (GET_MODE (XVECEXP (span,
> +								  0, i))),
> +				result.span_width));
> +	  gcc_assert (REG_P (XVECEXP (span, 0, i)));
> +	  gcc_assert (dwf_regno (XVECEXP (span, 0, i)) == result.reg + i);
> +	}
> +#endif
> +    }
> +
> +  return result;

> @@ -3135,7 +3267,8 @@ static unsigned int
>  execute_dwarf2_frame (void)
>  {
>    /* Different HARD_FRAME_POINTER_REGNUM might coexist in the same file.  */
> -  dw_frame_pointer_regnum = DWARF_FRAME_REGNUM (HARD_FRAME_POINTER_REGNUM);
> +  dw_frame_pointer_regnum = dwf_cfa_reg (gen_rtx_REG
> +					(Pmode, HARD_FRAME_POINTER_REGNUM));

If at all possible, avoid function name on one line and ( with first
argument on another one.  In the above case it can be easily avoided by
  dw_frame_pointer_regnum
    = dwf_cfa_reg (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM));

> +static void
> +build_breg_loc (struct dw_loc_descr_node **head, unsigned int regno)
> +{
> +  if (regno <= 31)
> +    add_loc_descr (head, new_loc_descr ((enum dwarf_location_atom)
> +		  (DW_OP_breg0 + regno),  0, 0));

Bad formatting, (DW_OP_breg0 should be below (enum
> +
> +  /* deal with the remaining registers in the span.  */

Capital letter on Deal

> +  for (int i = (reg.span - 2); i >= 0; i--)

Please remove the redundant inner () pair.
> --- a/gcc/dwarf2out.h
> +++ b/gcc/dwarf2out.h
> @@ -119,6 +119,38 @@ struct GTY(()) dw_fde_node {
>  };
>  
>  
> +/* This represents a register, in DWARF_FRAME_REGNUM space, for use in CFA
> +   definitions and expressions.
> +   Most architectures only need a single register number, but some (amdgcn)
> +   have pointers that span multiple registers.  DWARF permits arbitrary
> +   register sets but existing use-cases only require contiguous register
> +   sets, as represented here.  */
> +struct GTY(()) cfa_reg {
> +  unsigned int reg;
> +  unsigned short span;
> +  unsigned short span_width;  /* A.K.A. register mode size.  */
> +
> +  cfa_reg& set_by_dwreg (unsigned int r)
> +    {
> +      reg = r;
> +      span = 1;
> +      span_width = 0;  /* Unknown size (permitted when span == 1).  */
> +      return *this;
> +    }
> +
> +  bool operator== (const cfa_reg other) const

The normal C++ way would be (const cfa_reg &other), wouldn't it?
Or otherwise the const keyword doesn't make much sense.

> +    {
> +      return (reg == other.reg && span == other.span
> +	      && (span_width == other.span_width
> +		  || (span == 1
> +		      && (span_width == 0 || other.span_width == 0))));
> +    }
> +  bool operator!= (const cfa_reg other) const

Likewise.
Please add an empty line above operator!=

> +    {
> +      return !(*this == other);
> +    }
> +};
> +

Otherwise LGTM.

	Jakub