From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31813 invoked by alias); 6 Feb 2009 15:50:11 -0000 Received: (qmail 31794 invoked by uid 22791); 6 Feb 2009 15:50:08 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,SARE_SUB_GETRID,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 06 Feb 2009 15:50:03 +0000 Received: (qmail 18838 invoked from network); 6 Feb 2009 15:50:01 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 6 Feb 2009 15:50:01 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: Get rid of linux-thread-db.c:target_beneath Date: Fri, 06 Feb 2009 15:50:00 -0000 User-Agent: KMail/1.9.10 Cc: teawater References: <20090206030528.2E7A0EAEE0@pedro-laptop-dell> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200902061550.13149.pedro@codesourcery.com> X-IsSubscribed: yes 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: 2009-02/txt/msg00156.txt.bz2 On Friday 06 February 2009 04:56:58, teawater wrote: > But we still meet the problem that if a function of beneath target is > NULL, current target use find_target_beneath to get the beneath > function pointer is not easy and dangerous like process record meet, > right? linux-thread-db.c always sits on top of linux-nat.c currently, so it is safe to use in the cases I adjusted. When it isn't "safe", linux-thread-db.c already checks for NULL-ness. > What about make a interface to support safe beneath function? For now, you can check that the function pointer is NULL before calling it. If it is NULL, then try the next target beneath, etc. See target_attach, target_flash_erase, etc. You could do something similar in the record.c target. Things are cleaner on the target method implementation side if the target method takes a "this" pointer, like the one I'm adding to target_wait. If you do that, you get rid of the record_beneath_to_resume, record_beneath_to_wait, record_beneath_to_store_registers, etc. function pointer hacks you had. The wrinkle that has, is that your method ends up responsible for doing the default if no target implements the method. IMO, that would acceptible for now. But, it can be fixed. Now we're mixing up threads, and I was going to start doing / proposing this separately, but since you are asking...: There are diferent "kinds" of target methods. - those that are defaulted to something that throws an error say, target_resume calls "noprocess" by default. - some are always defaulted to something that returns a reasonable default, or just an ignore function say, to_fetch_registers, which defaults to "target_ignore". - some are optional and so default to NULL. Usually in these cases, the code in target.c checks for NULLness and continues looking up in the target beneath for a suitable function pointer. - some are implemented using inheritance when building up the current_target target (INHERIT in target.c). On these, you want to call current_target.foofunc. - some do the beneath lookup themselves to get away from this inheritance mechanism. This is all naturally, a mess. As we talked about before, a target method should default to calling the same method on the target beneath. In the bottom of the stack, you have a sentinel target (the dummy target) that should implement the default behaviour of the target method. This way, a target method is never null, so it is always safe to call the beneath method: target_foomethod (struct target_ops *ops, ...) { beneath = find_target_beneath (ops); return beneath->foofunc (beneath, ...); } See below an example of what I'm talking about. There a couple of methods were this may get tricky, but nothing unsurmountable. Notice how doing this also suggests that target.c/target.h could be half generated by a target.sh script, similarly to what we do with gdbarch.{c|h|sh}. There are few methods in the dummy target, that default to look up the default run target and run the method there. This suggests that if we change things like I suggested above, we need to either make all process_stratum targets implement those methods, do the lookup beneath stops there, or, make all process_stratum targets inherit (in the c++ sense) from inf-child.c or similar; otherwise, e.g, we could be connected to some remote-like target, while the method would be run on the native target. -- Pedro Alves --- gdb/target.c | 80 +++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 45 insertions(+), 35 deletions(-) Index: src/gdb/target.c =================================================================== --- src.orig/gdb/target.c 2009-02-06 14:41:50.000000000 +0000 +++ src/gdb/target.c 2009-02-06 15:20:36.000000000 +0000 @@ -376,6 +376,19 @@ default_get_ada_task_ptid (long lwp, lon return ptid_build (ptid_get_pid (inferior_ptid), lwp, tid); } +static ptid_t +default_to_wait (struct target_ops *ops, + ptid_t ptid, struct target_waitstatus *status) +{ + return (*ops->beneath->to_wait) (ops, ptid, status); +} + +static void +default_to_attach (struct target_ops *ops, char *args, int from_tty) +{ + (*ops->beneath->to_attach) (ops, args, from_tty); +} + /* Go through the target stack from top to bottom, copying over zero entries in current_target, then filling in still empty entries. In effect, we are doing class inheritance through the pushed target @@ -506,9 +519,11 @@ update_current_target (void) de_fault (to_close, (void (*) (int)) target_ignore); + de_fault (to_attach, default_to_attach); de_fault (to_post_attach, (void (*) (int)) target_ignore); + de_fault (to_wait, default_to_wait); de_fault (to_resume, (void (*) (ptid_t, int, enum target_signal)) noprocess); @@ -1848,31 +1863,22 @@ target_disconnect (char *args, int from_ ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status) { - struct target_ops *t; + struct target_ops *t = current_target.beneath; + ptid_t retval = (t->to_wait) (t, ptid, status); - for (t = current_target.beneath; t != NULL; t = t->beneath) + if (targetdebug) { - if (t->to_wait != NULL) - { - ptid_t retval = (*t->to_wait) (t, ptid, status); + char *status_string; - if (targetdebug) - { - char *status_string; - - status_string = target_waitstatus_to_string (status); - fprintf_unfiltered (gdb_stdlog, - "target_wait (%d, status) = %d, %s\n", - PIDGET (ptid), PIDGET (retval), - status_string); - xfree (status_string); - } - - return retval; - } + status_string = target_waitstatus_to_string (status); + fprintf_unfiltered (gdb_stdlog, + "target_wait (%d, status) = %d, %s\n", + PIDGET (ptid), PIDGET (retval), + status_string); + xfree (status_string); } - noprocess (); + return retval; } char * @@ -2529,6 +2535,19 @@ dummy_pid_to_str (struct target_ops *ops return normal_pid_to_str (ptid); } +static ptid_t +dummy_to_wait (struct target_ops *ops, + ptid_t ptid, struct target_waitstatus *status) +{ + noprocess (); +} + +void +dummy_to_attach (char *args, int from_tty) +{ + noprocess (); +} + /* Error-catcher for target_find_memory_regions */ static int dummy_find_memory_regions (int (*ignore1) (), void *ignore2) { @@ -2557,6 +2576,7 @@ init_dummy_target (void) (void (*)(struct target_ops *, char *, int))target_ignore; dummy_target.to_create_inferior = find_default_create_inferior; dummy_target.to_can_async_p = find_default_can_async_p; + dummy_target.to_wait = dummy_to_wait; dummy_target.to_is_async_p = find_default_is_async_p; dummy_target.to_supports_non_stop = find_default_supports_non_stop; dummy_target.to_pid_to_str = dummy_pid_to_str; @@ -2590,21 +2610,11 @@ target_close (struct target_ops *targ, i void target_attach (char *args, int from_tty) { - struct target_ops *t; - for (t = current_target.beneath; t != NULL; t = t->beneath) - { - if (t->to_attach != NULL) - { - t->to_attach (t, args, from_tty); - if (targetdebug) - fprintf_unfiltered (gdb_stdlog, "target_attach (%s, %d)\n", - args, from_tty); - return; - } - } - - internal_error (__FILE__, __LINE__, - "could not find a target to attach"); + struct target_ops *t = current_target.beneath; + (*t->to_attach) (t, args, from_tty); + if (targetdebug) + fprintf_unfiltered (gdb_stdlog, "target_attach (%s, %d)\n", + args, from_tty); } static void