From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127112 invoked by alias); 3 Jul 2015 16:37:01 -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 127095 invoked by uid 89); 3 Jul 2015 16:37:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 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; Fri, 03 Jul 2015 16:36:58 +0000 Received: from /spool/local by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 3 Jul 2015 17:36:55 +0100 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; Fri, 3 Jul 2015 17:36:52 +0100 X-MailFrom: uweigand@de.ibm.com X-RcptTo: gdb-patches@sourceware.org Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 270BC17D8042 for ; Fri, 3 Jul 2015 17:38:06 +0100 (BST) Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t63GaqPl34734244 for ; Fri, 3 Jul 2015 16:36:52 GMT Received: from d06av09.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t63GaqBu004503 for ; Fri, 3 Jul 2015 10:36:52 -0600 Received: from oc7340732750.ibm.com (icon-9-164-134-232.megacenter.de.ibm.com [9.164.134.232]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id t63GaphG004494; Fri, 3 Jul 2015 10:36:51 -0600 Received: by oc7340732750.ibm.com (Postfix, from userid 500) id E266B5BFD; Fri, 3 Jul 2015 18:36:50 +0200 (CEST) Subject: Re: [PATCH 4/5 v4] Tracepoint for ppc64. To: cole945@gmail.com (Wei-cheng Wang) Date: Fri, 03 Jul 2015 16:37:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org, cole945@gmail.com (Wei-cheng Wang) In-Reply-To: <1435422102-39438-4-git-send-email-cole945@gmail.com> from "Wei-cheng Wang" at Jun 28, 2015 12:21:41 AM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-Id: <20150703163650.E266B5BFD@oc7340732750.ibm.com> X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15070316-0021-0000-0000-0000047D7778 X-SW-Source: 2015-07/txt/msg00099.txt.bz2 Wei-cheng Wang wrote: > Ulrich Weigand wrote: > > Well, but we know the logic the stub uses. For example, we know that > > we certainly cannot install a fast tracepoint in any shared library code, > > since the jump pad will definitely be too far away. We can check for > > this condition here. (We could also check for tracepoints in executables > > that have a text section larger than 32 MB ...) > > Now in this path, ppc_fast_tracepoint_valid_at will check the distance > and return 0 (invalid) if ADDR is too far from jumppad. > > However, if a tracepoint was pending and later found it's not valid, > it will cause an internal-error. See remote.c > > if (gdbarch_fast_tracepoint_valid_at (target_gdbarch (), > tpaddr, &isize, NULL)) > xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":F%x", > isize); > else > /* If it passed validation at definition but fails now, > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > something is very wrong. */ > internal_error (__FILE__, __LINE__, _("Fast tracepoint not " > "valid during download")); > > If the tracepoint is pending at definition, it won't be checked at all. > > /* Fast tracepoints may have additional restrictions on location. */ > if (!pending && type_wanted == bp_fast_tracepoint) > ^^^^^^^^ > { > for (ix = 0; VEC_iterate (linespec_sals, canonical.sals, ix, iter); ++ix) > check_fast_tracepoint_sals (gdbarch, &iter->sals); > } > > Maybe we use "error" instead of "internal_error"? Huh. Shouldn't the location be re-checked in breakpoint.c once the tracepoint is re-set from "pending" to active? It seems there's a call to gdbarch_fast_tracepoint_valid_at missing at that point. (If the gdbarch_fast_tracepoint_valid_at fails then, we probably should emit an error and either leave the tracepoint pending or else convert it to a regular tracepoint at that point.) > A new function (gdbserver target op), ppc_get_thread_area, is implemented > used for the jump pad lock object (collecting_t). Please have a look. This looks fine to me. > Changing timeout in tspeed.exp is reverted in this patch. Recently I > tested the patch on gcc112 server and timeout should be 540 instead of 360 > in order to pass tspeed.exp. I am not sure what's the proper value for timeout. > Maybe someone will test it in another environment, and 540 is not enough again. OK. Do we know why the test case is taking so long? Is this simply to be expected, or is the slowness due to some hidden issue that should be fixed? > BTW, in this patch, unlock is implemented by simply writing 0 instead > of a full atomic-swap. It that ok? > > /* Prepare collecting_t object for lock. */ > p += GEN_STORE (p, 3, 1, min_frame + 37 * rsz); > p += GEN_STORE (p, tp_reg, 1, min_frame + 38 *rsz); > /* Set R5 to collecting object. */ > p += GEN_ADDI (p, 5, 1, 37 * rsz); > > p += gen_atomic_xchg (p, lockaddr, 0, 5); > > /* Call to collector. */ > p += gen_call (p, collector); > > /* Simply write 0 to release the lock. */ > p += gen_limm (p, 3, lockaddr); > p += gen_limm (p, 4, 0); > p += GEN_LWSYNC (p); > p += GEN_STORE (p, 4, 3, 0); This should be OK. However, we need to take care to use appropriate syncs when taking the lock as well. > + /* > + 1: lwsync > + 2: lwarx TMP, 0, LOCK > + cmpwi TMP, OLD > + bne 1b > + stwcx. NEW, 0, LOCK > + bne 2b */ In particular, in order to guarantee proper aqcuire semantics, there needs to be an lwsync (or isync) *after* the loop here. On the other hand, you do not need to loop back to the lwsync at the beginning. In fact, for a normal lock-acquire sequence you would not need the lwsync before the loop at all. However, in this specific case, since the lock value is itself a pointer to a data structure, you *do* want to have a lwsync between storing that data structure and taking the lock. It's probably best to move the lwsync out of this subroutine and place the two lwsyncs in the caller. Also, a couple of comments from an earlier review still apply: > +#if __PPC64__ Please use #ifdef __powerpc64__ throughout. > +static int > +gen_limm (uint32_t *buf, int reg, uint64_t imm) > +{ > + uint32_t *p = buf; > + > + if ((imm + 32768) < 65536) > + { > + /* li reg, imm[7:0] */ > + p += GEN_LI (p, reg, imm); > + } > + else if ((imm >> 16) == 0) > + { > + /* li reg, 0 > + ori reg, reg, imm[15:0] */ > + p += GEN_LI (p, reg, 0); > + p += GEN_ORI (p, reg, reg, imm); > + } > + else if ((imm >> 32) == 0) > + { > + /* lis reg, imm[31:16] > + ori reg, reg, imm[15:0] */ > + p += GEN_LIS (p, reg, (imm >> 16) & 0xffff); > + p += GEN_ORI (p, reg, reg, imm & 0xffff); This is still incorrect if bit 31 of imm is set. (The lis will fill the 32 high bits of the register with 1 instead 0 in that case.) > +static void > +ppc_relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc) > +{ [...] > + else if ((PPC_BO (insn) & 0x14) == 0x4 || (PPC_BO (insn) & 0x14) == 0x10) > + { > + newrel -= 4; > + > + /* Out of range. Cannot relocate instruction. */ > + if (newrel >= (1 << 25) || newrel < -(1 << 25)) > + return; > + > + if ((PPC_BO (insn) & 0x14) == 0x4) > + insn ^= (1 << 24); > + else if ((PPC_BO (insn) & 0x14) == 0x10) > + insn ^= (1 << 22); > + > + /* Jump over the unconditional branch. */ > + insn = (insn & ~0xfffc) | 0x8; > + write_inferior_memory (*to, (unsigned char *) &insn, 4); > + *to += 4; > + > + /* Build a unconditional branch and copy LK bit. */ > + insn = (18 << 26) | (0x3fffffc & newrel) | (insn & 0x3); > + write_inferior_memory (*to, (unsigned char *) &insn, 4); > + *to += 4; > + > + return; > + } > + else This should check for (PPC_BO (insn) & 0x14) == 0. Note that (PPC_BO (insn) & 0x14) == 0x14 is the "branch always" case, which should be replaced simply by an unconditional branch. > + { > + uint32_t bdnz_insn = (16 << 26) | (0x10 << 21) | 12; > + uint32_t bf_insn = (16 << 26) | (0x4 << 21) | 8; > + > + newrel -= 12; That should be -= 8, shouldn't it? > +static void > +ppc64_emit_stack_adjust (int n) > +{ > + uint32_t buf[4]; > + uint32_t *p = buf; > + > + n = n << 3; > + if ((n >> 7) != 0) > + { > + emit_error = 1; > + return; > + } > + > + p += GEN_ADDI (p, 30, 30, n); ADDI supports up to 15-bit unsigned operands, so this seems overly strict. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com