Re: [PATCH 2/3] Aarch64: Detect FP regs in signal frames

[Simon Marchi <simark at simark dot ca>]

On 2018-09-17 8:53 a.m., Alan Hayward wrote:
> Both the VFP and SVE registers may be contained within the reserved space of
> the sigcontext and can be found by seraching for MAGIC values. Detect these
> and add the registers (including pseudos) to the trad frame cache.

Hi Alan,

I don't know by heart all the intricacies of aarch64/sve, so I wouldn't spot
a subtle mistake, but overall this made sense to me.  I noted a few formatting
nits.  Unless somebody with more AArche64 knowledge wants to take a look, I'd say
this is ok to go in with the nits fixed.

I was a bit surprised that it was necessary to provide the value even for pseudo registers.
I would have expected that only raw registers would be sufficient, and that
tramp_frame_prev_register would reconstruct the value of pseudo ones based on the value
of raw ones.  But there's nothing dealing with pseudo registers in there, so I guess
it's necessary to provide all of them.

> +/* Read an aarch64_ctx, returning the magic value, and setting size to the size,
> +   or return 0 on error.  */

"and setting *SIZE to the size"

> @@ -145,14 +190,20 @@ aarch64_linux_sigframe_init (const struct tramp_frame *self,
>  			     struct trad_frame_cache *this_cache,
>  			     CORE_ADDR func)
>  {
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>    CORE_ADDR sp = get_frame_register_unsigned (this_frame, AARCH64_SP_REGNUM);
> -  CORE_ADDR sigcontext_addr =
> -    sp
> -    + AARCH64_RT_SIGFRAME_UCONTEXT_OFFSET
> -    + AARCH64_UCONTEXT_SIGCONTEXT_OFFSET;
> -  int i;
> -
> -  for (i = 0; i < 31; i++)
> +  CORE_ADDR sigcontext_addr = sp + AARCH64_RT_SIGFRAME_UCONTEXT_OFFSET
> +			      + AARCH64_UCONTEXT_SIGCONTEXT_OFFSET;

Use parentheses around an expression when breaking it:

  CORE_ADDR sigcontext_addr = (sp + AARCH64_RT_SIGFRAME_UCONTEXT_OFFSET
			       + AARCH64_UCONTEXT_SIGCONTEXT_OFFSET);

Search for "Insert extra parentheses" in:

  https://www.gnu.org/prep/standards/standards.html

There's at least one other occurrence of this, so maybe go over the patch quickly
to fix other cases.

> +	case AARCH64_EXTRA_MAGIC:
> +	  {
> +	    /* Extra is always the penultimate section in reserved and points to
> +	       an block of memory block of sections.  Reset the address to the

"an block".  Also, not sure if "a block of memory block of sections" is really what you meant,
or just "a memory block of sections"?

> +	       extra section and continue looking for more sections.  */
> +	    gdb_byte buf[8];
> +
> +	    if (target_read_memory (section + AARCH64_EXTRA_DATAP_OFFSET,
> +				    buf, 8) != 0)
> +	      {
> +		section += size;
> +		break;
> +	      }
> +
> +	    section = extract_unsigned_integer (buf, 8, byte_order);
> +	    break;
> +	  }
> +
> +	default:
> +	  break;
> +	}
> +    }
> +
> +  /* If there was an SVE section, record the location of all the FP regs.  */
> +  if (sve_regs)

sve_regs != 0

> +    {
> +      CORE_ADDR offset;
> +
> +      for (int i = 0; i < 32; i++)
> +	{
> +	  offset = sve_regs + (i * tdep->vq * 16);
> +	  trad_frame_set_reg_addr (this_cache, AARCH64_SVE_Z0_REGNUM + i,
> +				   offset);
> +	  trad_frame_set_reg_addr (this_cache,
> +				   num_regs + AARCH64_SVE_V0_REGNUM + i,
> +				   offset);
> +	  trad_frame_set_reg_addr (this_cache, num_regs + AARCH64_Q0_REGNUM + i,
> +				   offset);
> +	  trad_frame_set_reg_addr (this_cache, num_regs + AARCH64_D0_REGNUM + i,
> +				   offset);
> +	  trad_frame_set_reg_addr (this_cache, num_regs + AARCH64_S0_REGNUM + i,
> +				   offset);
> +	  trad_frame_set_reg_addr (this_cache, num_regs + AARCH64_H0_REGNUM + i,
> +				   offset);
> +	  trad_frame_set_reg_addr (this_cache, num_regs + AARCH64_B0_REGNUM + i,
> +				   offset);
> +	}
> +
> +      offset = sve_regs + AARCH64_SVE_CONTEXT_P_REGS_OFFSET (tdep->vq);
> +      for (int i = 0; i < 16; i++)
> +	trad_frame_set_reg_addr (this_cache, AARCH64_SVE_P0_REGNUM + i,
> +				 offset + (i * tdep->vq * 2));
> +
> +      offset = sve_regs + AARCH64_SVE_CONTEXT_FFR_OFFSET (tdep->vq);
> +      trad_frame_set_reg_addr (this_cache, AARCH64_SVE_FFR_REGNUM, offset);
> +    }
> +
> +  /* If there was a fpsimd section, record the location of all the VFP regs.  */
> +  if (fpsimd)

fpsimd != 0

Thanks,

Simon