This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] gdbserver/s390: Add fast tracepoint support.
- From: Andreas Arnez <arnez at linux dot vnet dot ibm dot com>
- To: Marcin KoÅcielnicki <koriakin at 0x04 dot net>
- Cc: gdb-patches at sourceware dot org, antoine dot tremblay at ericsson dot com
- Date: Mon, 14 Mar 2016 17:19:23 +0100
- Subject: Re: [PATCH v2] gdbserver/s390: Add fast tracepoint support.
- Authentication-results: sourceware.org; auth=none
- References: <wwokoabcpxu3 dot fsf at ericsson dot com> <1457088025-3118-1-git-send-email-koriakin at 0x04 dot net>
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