From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13807 invoked by alias); 2 Jun 2006 08:44:05 -0000 Received: (qmail 13784 invoked by uid 22791); 2 Jun 2006 08:44:02 -0000 X-Spam-Check-By: sourceware.org Received: from nitzan.inter.net.il (HELO nitzan.inter.net.il) (192.114.186.20) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 02 Jun 2006 08:43:48 +0000 Received: from HOME-C4E4A596F7 (IGLD-80-230-33-177.inter.net.il [80.230.33.177]) by nitzan.inter.net.il (MOS 3.7.3-GA) with ESMTP id DOD68689 (AUTH halo1); Fri, 2 Jun 2006 11:42:57 +0300 (IDT) Date: Fri, 02 Jun 2006 08:44:00 -0000 Message-Id: From: Eli Zaretskii To: gdb-patches@sourceware.org CC: Nathan Sidwell In-reply-to: <20060601211159.GA557@nevyn.them.org> (message from Daniel Jacobowitz on Thu, 1 Jun 2006 17:11:59 -0400) Subject: Re: patch for invalid hw breakpoints Reply-to: Eli Zaretskii References: <447EE9A8.4050800@codesourcery.com> <20060601172639.GA25709@nevyn.them.org> <447F27BC.6030808@codesourcery.com> <20060601180321.GA26791@nevyn.them.org> <20060601211159.GA557@nevyn.them.org> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-06/txt/msg00016.txt.bz2 > Date: Thu, 1 Jun 2006 17:11:59 -0400 > From: Daniel Jacobowitz > Cc: Nathan Sidwell , gdb-patches@sourceware.org > > > Is it really clean for remote.c to throw an internal error? I always > > thought remote.c implemented an API similar to ptrace, so it should > > behave like ptrace, i.e. return an error code, not throw exceptions. > > Normally it would; this is an internal error, i.e. a failed consistency > check. Well, it calls error(), but I think it really qualifies as an > internal error. Why cannot it return an error code instead? > Anyway, do you think it's sensible for a "target_remove_watchpoint" > method to be called on something that is not an inserted watchpoint? > I'd think no. It could mess up reference counts, for instance. The problem is, the high-level GDB interface to target-side watchpoint support currently assumes that the target will cope with such problems. breakpoint.c doesn't know nor care about such ``unimportant'' details as how many addresses the target needs to watch for a given data object on the value chain. The requirement from the target is to maintain whatever data it needs to track the watchpoint related resources at all times, and silently cope with seemingly unreasonable requests issued by the relatively blissful high-level code in breakpoint.c. For that, no target should ever throw exceptions when GDB tries to do something unreasonable with watchpoints. For example, in the specific example of reference counts, the target end should increment and decrement the counts even if it doesn't actually insert/remove the watchpoint at the specified address. If we are about to change this basic arrangement, i.e. if we want to keep some of the information on the application level, I fear we will eventually need to redesign the whole interface. Meanwhile, I really don't like the fact that remote.c throws an internal error in situations that don't require that. To me, internal error means a situation akin to SIGSEGV: something is dead wrong, but the code has no means of figuring out what's that. I don't think we are talking about such a situation. On the contrary, the code knows _exactly_ what went wrong: it was called to provide an unsupported functionality. The evident assumption that it will _never_ be called more than once with unsupported requests is simply wrong: nobody said that the application side is smart enough to always avoid that. And even if such an assumption was valid, it is not up to remote.c to decide that an internal error is in order, because the impossible situation happened in higher-level GDB code, not in remote.c. I think it is not a modular design to have code that throws an exception below the level where an impossible situation happened. > > If solving this in remote.c seems unclean for some reason (I don't > > think so, but that's me), how about adding to the watchpoint data > > structure a flag for each low-level watchpoint we insert, and storing > > there whether it was actually inserted? The code that removes > > watchpoints could then consult that flag and refrain from removing a > > non-inserted watch. Does this make sense? > > That seems fine to me. If you recall, long ago I was planning to split > this up to have a "low level breakpoint" (struct bp_location) for each > value in the chain, and bp_location already has the inserted flag. But > I never had time to finish it. This would be a less intrusive way of > accomplishing about the same thing. Indeed. But I think remote.c shouldn't throw an internal error in such cases.