From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32461 invoked by alias); 1 Nov 2008 18:54:52 -0000 Received: (qmail 32395 invoked by uid 22791); 1 Nov 2008 18:54:51 -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.31) with ESMTP; Sat, 01 Nov 2008 18:54:15 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id BDAEE2A9662; Sat, 1 Nov 2008 14:54:12 -0400 (EDT) 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 BC5Puj-KhDPQ; Sat, 1 Nov 2008 14:54:12 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 62CB52A963B; Sat, 1 Nov 2008 14:54:12 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 32056E7ACD; Sat, 1 Nov 2008 11:54:10 -0700 (PDT) Date: Sat, 01 Nov 2008 18:54:00 -0000 From: Joel Brobecker To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: [patch] ia64: Fix breakpoints memory shadow Message-ID: <20081101185410.GB15606@adacore.com> References: <20081028172816.GA1284@host0.dyn.jankratochvil.net> <20081029210242.GA3635@adacore.com> <20081030144841.GA26606@host0.dyn.jankratochvil.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20081030144841.GA26606@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/msg00001.txt.bz2 > I agree your proposal would be better but we have to comply with the current > `struct bp_target_info' layout which is being intepreted outside of > ia64-tdep.c - in breakpoint_restore_shadows. > > If we would like to store the whole bundle to SHADOW_CONTENTS we would have to > store already the base address (`address & ~0x0f') into PLACED_ADDRESS. In > such case there is no other place where to store SLOTNUM (`adress & 0x0f', > value in the range <0..2>). We need to know SLOTNUM in > ia64_memory_remove_breakpoint. Aie aie aie, that's true. I was also looking at the idea of stuffing the slot number in the first 5 bits of the bundle, but that wouldn't work either, as breakpoint_restore_shadows can restore those bits as well. There is one alternative which is to define a gdbarch restore_shadow, but your approach does work after all, so let's not complexify the rest of GDB just for ia64. Can we maybe expand the comment explaining why we can't save the whole bundle in the shadow buffer? > const unsigned char * > ia64_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr) > { > - static unsigned char breakpoint[] = > - { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }; > - *lenptr = sizeof (breakpoint); > -#if 0 > - *pcptr &= ~0x0f; > + static unsigned char breakpoint[BUNDLE_LEN]; > + int slotnum = (int) (*pcptr & 0x0f) / SLOT_MULTIPLIER; > + > + if (slotnum > 2) > + error (_("Can't insert breakpoint for slot numbers greater than 2.")); > + > +#if BREAKPOINT_MAX < BUNDLE_LEN > +# error "BREAKPOINT_MAX < BUNDLE_LEN" > #endif > + > + *lenptr = BUNDLE_LEN - 2; > + > return breakpoint; > } I think there is a piece missing in this change. Looks like breakpoint is no longer initialized. I think that what you need to do is read the instruction bundle from memory, and insert the breakpoint instruction, and then copy you BUNDLE_LEN - 2 bytes into the breakpoint buffer. The reason why we didn't trip over this, I believe, is because this routine is only used when using the default insert/remove bp routines, or when using the remote protocol. It's also used from monitor.c. Also, the check for BUNDLE_LEN against BREAKPOINT_MAX is slightly over-pessimistic. Can you move it somewhere at the beginning of the file (this assumption is required in other parts of the code), and compare BREAKPOINT_MAX against the actual size of data saved (BUNDLE_LEN -2)? The rest looks OK to me. -- Joel