Re: [PATCH v2] gdbserver/s390: Add fast tracepoint support.

[Andreas Arnez <arnez@linux.vnet.ibm.com>]

On Fri, Mar 04 2016, Marcin Kościelnicki wrote:

> Fast tracepoints will only work on 6-byte intructions, and assume at least
> a z900 CPU.  s390 also has 4-byte jump instructions, which also work on
> pre-z900, but their range is limitted to +-64kiB, which is not very useful
> (and wouldn't work at all with current jump pad allocation).
>
> There's a little problem with s390_relocate_instruction function: it
> converts BRAS/BRASL instructions to LARL of the return address + JG
> to the target address.  On 31-bit, this sets the high bit of the target
> register to 0, while BRAS/BRASL would set it to 1.  While this is not
> a problem when the result is only used to address memory, it could
> possibly break something that expects to compare such addresses for
> equality without first masking the bit off.  In particular, I'm not sure
> whether leaving the return address high bit unset is ABI-compliant
> (could confuse some unwinder?).  If that's a problem, it could be fixed
> by handling it in the jump pad (since at that point we can just modify
> the GPRs in the save area without having to worry about preserving
> CCs and only having that one GPR to work with - I'm not sure if it's
> even possible to set the high bit with such constraints).
>
> gdb/gdbserver/ChangeLog:
>
> 	* Makefile.in: Add s390 IPA files.
> 	* configure.srv: Build IPA for s390.
> 	* linux-s390-ipa.c: New file.
> 	* linux-s390-low.c: New includes - inttypes.h and linux-s390-tdesc.h.
> 	(init_registers_s390_linux32): Move declaration to linux-s390-tdesc.h.
> 	(tdesc_s390_linux32): Ditto.
> 	(init_registers_s390_linux32v1): Ditto.
> 	(tdesc_s390_linux32v1): Ditto.
> 	(init_registers_s390_linux32v2): Ditto.
> 	(tdesc_s390_linux32v2): Ditto.
> 	(init_registers_s390_linux64): Ditto.
> 	(tdesc_s390_linux64): Ditto.
> 	(init_registers_s390_linux64v1): Ditto.
> 	(tdesc_s390_linux64v1): Ditto.
> 	(init_registers_s390_linux64v2): Ditto.
> 	(tdesc_s390_linux64v2): Ditto.
> 	(init_registers_s390_te_linux64): Ditto.
> 	(tdesc_s390_te_linux64): Ditto.
> 	(init_registers_s390_vx_linux64): Ditto.
> 	(tdesc_s390_vx_linux64): Ditto.
> 	(init_registers_s390_tevx_linux64): Ditto.
> 	(tdesc_s390_tevx_linux64): Ditto.
> 	(init_registers_s390x_linux64): Ditto.
> 	(tdesc_s390x_linux64): Ditto.
> 	(init_registers_s390x_linux64v1): Ditto.
> 	(tdesc_s390x_linux64v1): Ditto.
> 	(init_registers_s390x_linux64v2): Ditto.
> 	(tdesc_s390x_linux64v2): Ditto.
> 	(init_registers_s390x_te_linux64): Ditto.
> 	(tdesc_s390x_te_linux64): Ditto.
> 	(init_registers_s390x_vx_linux64): Ditto.
> 	(tdesc_s390x_vx_linux64): Ditto.
> 	(init_registers_s390x_tevx_linux64): Ditto.
> 	(tdesc_s390x_tevx_linux64): Ditto.
> 	(have_hwcap_s390_vx): New static variable.
> 	(s390_arch_setup): Fill have_hwcap_s390_vx.
> 	(s390_get_thread_area): New function.
> 	(s390_ft_entry_gpr_esa): New const.
> 	(s390_ft_entry_gpr_zarch): New const.
> 	(s390_ft_entry_misc): New const.
> 	(s390_ft_entry_fr): New const.
> 	(s390_ft_entry_vr): New const.
> 	(s390_ft_main_31): New const.
> 	(s390_ft_main_64): New const.
> 	(s390_ft_exit_fr): New const.
> 	(s390_ft_exit_vr): New const.
> 	(s390_ft_exit_misc): New const.
> 	(s390_ft_exit_gpr_esa): New const.
> 	(s390_ft_exit_gpr_zarch): New const.
> 	(append_insns): New function.
> 	(s390_relocate_instruction): New function.
> 	(s390_install_fast_tracepoint_jump_pad): New function.
> 	(s390_get_min_fast_tracepoint_insn_len): New function.
> 	(s390_get_ipa_tdesc_idx): New function.
> 	(struct linux_target_ops): Wire in the above functions.
> 	(initialize_low_arch) [!__s390x__]: Don't initialize s390x tdescs.
> 	* linux-s390-tdesc.h: New file.

This is OK, apart from some minor nits (see below).  Note that we
usually say "likewise" instead of "ditto".

> +static int
> +s390_relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc, int is_64)
> +{

[...]

> +
> +	  /* XXX: this is not fully correct.  In 31-bit mode, LARL will write
> +	     an address with the top bit 0, while BRAS/BRASL will write it
> +	     with top bit 1.  It should not matter much, since linux compilers
> +	     use BR and not BSM to return from functions, but it could confuse
> +	     some poor stack unwinder.  */

We'll probably keep the logic like that anyway unless it's found to
cause real problems, so better replace the "XXX:" by a mere "Note:".
Also, "linux" should be "GNU/Linux".

> +
> +	  /* We'll now be writing a JG.  */
> +	  mode = 2;
> +	  buf[0] = 0xc0;
> +	  buf[1] = 0xf4;
> +	  ilen = 6;
> +	}
> +
> +      /* Compute the new offset and write it to the buffer.  */
> +      loffset = target - *to;
> +      loffset >>= 1;
> +
> +      if (mode == 1)
> +	{
> +	  int16_t soffset = loffset;
> +	  if (soffset != loffset)
> +	    return 1;
> +	  memcpy (buf+2, &soffset, 2);

"buf+2" -> "buf + 2".

> +	}
> +      else if (mode == 2)
> +	{
> +	  int32_t soffset = loffset;
> +	  if (soffset != loffset && is_64)
> +	    return 1;
> +	  memcpy (buf+2, &soffset, 4);

Same here.

[...]

> +static int
> +s390_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint,
> +				       CORE_ADDR tpaddr,
> +				       CORE_ADDR collector,
> +				       CORE_ADDR lockaddr,
> +				       ULONGEST orig_size,
> +				       CORE_ADDR *jump_entry,
> +				       CORE_ADDR *trampoline,
> +				       ULONGEST *trampoline_size,
> +				       unsigned char *jjump_pad_insn,
> +				       ULONGEST *jjump_pad_insn_size,
> +				       CORE_ADDR *adjusted_insn_addr,
> +				       CORE_ADDR *adjusted_insn_addr_end,
> +				       char *err)
> +{
> +  int i;
> +  int64_t loffset;
> +  int32_t offset;
> +  unsigned char jbuf[6] = { 0xc0, 0xf4, 0, 0, 0, 0 };	/* jg ... */
> +  CORE_ADDR buildaddr = *jump_entry;
> +#ifdef __s390x__
> +  struct regcache *regcache = get_thread_regcache (current_thread, 0);
> +  int is_64 = register_size (regcache->tdesc, 0) == 8;
> +  int is_zarch = is_64 || have_hwcap_s390_high_gprs;
> +  int has_vx = have_hwcap_s390_vx;
> +#else
> +  int is_64 = 0, is_zarch = 0, has_vx = 0;
> +#endif
> +  CORE_ADDR literals[4] = {
> +    tpaddr,
> +    tpoint,
> +    collector,
> +    lockaddr,
> +  };
> +
> +  /* First, store the GPRs.  */
> +  if (!is_zarch)
> +    append_insns (&buildaddr, sizeof s390_ft_entry_gpr_esa, s390_ft_entry_gpr_esa);
> +  else
> +    append_insns (&buildaddr, sizeof s390_ft_entry_gpr_zarch, s390_ft_entry_gpr_zarch);

Lines too long.  Also, non-reversed logic might be a bit clearer here
and in further such conditionals below.

[...]

> +
> +  /* Restore the GPRs.  */
> +  if (!is_zarch)
> +    append_insns (&buildaddr, sizeof s390_ft_exit_gpr_esa, s390_ft_exit_gpr_esa);
> +  else
> +    append_insns (&buildaddr, sizeof s390_ft_exit_gpr_zarch, s390_ft_exit_gpr_zarch);

Lines too long.

Otherwise the patch looks good to me.

Thanks,
Andreas