From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24134 invoked by alias); 25 Nov 2008 06:13:24 -0000 Received: (qmail 23964 invoked by uid 22791); 25 Nov 2008 06:13:23 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 25 Nov 2008 06:12:48 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 734BF2A964C; Tue, 25 Nov 2008 01:12:38 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id buyOGKzUDV9C; Tue, 25 Nov 2008 01:12:38 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 284842A9648; Tue, 25 Nov 2008 01:12:38 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id C12AAE7ACD; Mon, 24 Nov 2008 22:12:35 -0800 (PST) Date: Tue, 25 Nov 2008 17:49:00 -0000 From: Joel Brobecker To: Jan Kratochvil Cc: Eli Zaretskii , gdb-patches@sourceware.org Subject: Re: [patch] ia64: Fix breakpoints memory shadow Message-ID: <20081125061235.GC3946@adacore.com> References: <20081028172816.GA1284@host0.dyn.jankratochvil.net> <20081029210242.GA3635@adacore.com> <20081030144841.GA26606@host0.dyn.jankratochvil.net> <20081101185410.GB15606@adacore.com> <20081111131726.GA3272@host0.dyn.jankratochvil.net> <20081113053100.GL5112@adacore.com> <20081120144936.GA25926@host0.dyn.jankratochvil.net> <20081122181525.GG4318@adacore.com> <20081122235450.GA17397@host0.dyn.jankratochvil.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20081122235450.GA17397@host0.dyn.jankratochvil.net> User-Agent: Mutt/1.4.2.2i 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 X-SW-Source: 2008-11/txt/msg00684.txt.bz2 > Still I rather found another defect compared to mem-break.c in the ia64 > breakpoints code. It is another problem unrelated to the original intention > of this ia64 patch. Jan, please don't take it personally, I really appreciate the way you are trying to make your patch better and better. But since this was unrelated, I really wished that you committed the first part as approved, and then sent a followup patch that dealt with that part on its own. With the approach you took, I had to fish out the previous version of the patch, and then compare the two patches to see exactly what changed. And since I've always found that a diff of diff is some kind of mental gymnastics, and the whole process is more work than just looking at the subsequent patch, I had to delay the review until I had 30min of quiet time. Anyway, back to the one important change: > - memcpy (&instr, bp_tgt->shadow_contents, sizeof instr); > - replace_slotN_contents (bundle, instr, slotnum); > + instr_breakpoint = slotN_contents (bundle_mem, slotnum); > + if (instr_breakpoint != IA64_BREAKPOINT) > + return -1; I think we need to give the user a way to understand the error message that returning -1 will cause. Otherwise, it's going to be hard for the user to have a clue of why the debugger failed to de-insert the breakpoint. I propose a warning saying something like this: cannot remove breakpoint at address %s, no break instruction at such address. Cheers, -- Joel