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

Andreas Arnez
2016-03-14T16:19:32Z

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