From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 60532 invoked by alias); 14 Mar 2016 16:19:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 60501 invoked by uid 89); 14 Mar 2016 16:19:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=int32_t, Ditto, ccs, xxx X-HELO: e06smtp15.uk.ibm.com Received: from e06smtp15.uk.ibm.com (HELO e06smtp15.uk.ibm.com) (195.75.94.111) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Mon, 14 Mar 2016 16:19:30 +0000 Received: from localhost by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 14 Mar 2016 16:19:27 -0000 Received: from d06dlp01.portsmouth.uk.ibm.com (9.149.20.13) by e06smtp15.uk.ibm.com (192.168.101.145) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Mon, 14 Mar 2016 16:19:25 -0000 X-IBM-Helo: d06dlp01.portsmouth.uk.ibm.com X-IBM-MailFrom: arnez@linux.vnet.ibm.com X-IBM-RcptTo: gdb-patches@sourceware.org Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 9F32017D805F for ; Mon, 14 Mar 2016 16:19:55 +0000 (GMT) Received: from d06av03.portsmouth.uk.ibm.com (d06av03.portsmouth.uk.ibm.com [9.149.37.213]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u2EGJPE163570114 for ; Mon, 14 Mar 2016 16:19:25 GMT Received: from d06av03.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av03.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u2EGJN3h030640 for ; Mon, 14 Mar 2016 10:19:24 -0600 Received: from oc1027705133.ibm.com (dyn-9-152-212-180.boeblingen.de.ibm.com [9.152.212.180]) by d06av03.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id u2EGJNQD030611 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 14 Mar 2016 10:19:23 -0600 From: Andreas Arnez To: Marcin =?utf-8?Q?Ko=C5=9Bcielnicki?= Cc: gdb-patches@sourceware.org, antoine.tremblay@ericsson.com Subject: Re: [PATCH v2] gdbserver/s390: Add fast tracepoint support. References: <1457088025-3118-1-git-send-email-koriakin@0x04.net> Date: Mon, 14 Mar 2016 16:19:00 -0000 In-Reply-To: <1457088025-3118-1-git-send-email-koriakin@0x04.net> ("Marcin \=\?utf-8\?Q\?Ko\=C5\=9Bcielnicki\=22's\?\= message of "Fri, 4 Mar 2016 11:40:25 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16031416-0021-0000-0000-000020AB8EA5 X-IsSubscribed: yes X-SW-Source: 2016-03/txt/msg00214.txt.bz2 On Fri, Mar 04 2016, Marcin Ko=C5=9Bcielnicki 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 =3D 2; > + buf[0] =3D 0xc0; > + buf[1] =3D 0xf4; > + ilen =3D 6; > + } > + > + /* Compute the new offset and write it to the buffer. */ > + loffset =3D target - *to; > + loffset >>=3D 1; > + > + if (mode =3D=3D 1) > + { > + int16_t soffset =3D loffset; > + if (soffset !=3D loffset) > + return 1; > + memcpy (buf+2, &soffset, 2); "buf+2" -> "buf + 2". > + } > + else if (mode =3D=3D 2) > + { > + int32_t soffset =3D loffset; > + if (soffset !=3D 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] =3D { 0xc0, 0xf4, 0, 0, 0, 0 }; /* jg ... */ > + CORE_ADDR buildaddr =3D *jump_entry; > +#ifdef __s390x__ > + struct regcache *regcache =3D get_thread_regcache (current_thread, 0); > + int is_64 =3D register_size (regcache->tdesc, 0) =3D=3D 8; > + int is_zarch =3D is_64 || have_hwcap_s390_high_gprs; > + int has_vx =3D have_hwcap_s390_vx; > +#else > + int is_64 =3D 0, is_zarch =3D 0, has_vx =3D 0; > +#endif > + CORE_ADDR literals[4] =3D { > + tpaddr, > + tpoint, > + collector, > + lockaddr, > + }; > + > + /* First, store the GPRs. */ > + if (!is_zarch) > + append_insns (&buildaddr, sizeof s390_ft_entry_gpr_esa, s390_ft_entr= y_gpr_esa); > + else > + append_insns (&buildaddr, sizeof s390_ft_entry_gpr_zarch, s390_ft_en= try_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_exi= t_gpr_zarch); Lines too long. Otherwise the patch looks good to me. Thanks, Andreas