From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25133 invoked by alias); 14 Jan 2014 18:27:55 -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 25121 invoked by uid 89); 14 Jan 2014 18:27:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 14 Jan 2014 18:27:51 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s0EIRoWm029607 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 14 Jan 2014 13:27:50 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s0EIRm0i005642; Tue, 14 Jan 2014 13:27:49 -0500 Message-ID: <52D581A4.8020007@redhat.com> Date: Tue, 14 Jan 2014 18:27:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Tom Tromey CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Fix "is a record target open" checks. References: <1389640367-5571-1-git-send-email-tromey@redhat.com> <1389640367-5571-3-git-send-email-tromey@redhat.com> <52D523FD.8060205@redhat.com> <8738kqy40l.fsf@fleche.redhat.com> In-Reply-To: <8738kqy40l.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-01/txt/msg00454.txt.bz2 On 01/14/2014 03:30 PM, Tom Tromey wrote: > Pedro> +/* See record-full.h. */ > Pedro> + > Pedro> +int > Pedro> +record_full_is_used (void) > Pedro> +{ > Pedro> + struct target_ops *t; > Pedro> + > Pedro> + t = find_record_target (); > Pedro> + return (t != NULL > Pedro> + && (t->to_shortname == record_full_shortname > Pedro> + || t->to_shortname == record_full_core_shortname)); > > Could this check against record_full_*ops directly rather than checking > the shortname? Oh yes it could. I don't really know why I didn't think of that... Adjusted. > I'm curious about the rationale for this choice ENOCAFFEINE I guess. :-) > Pedro> +void > Pedro> +record_preopen (void) > Pedro> +{ > > Need a "/* See record.h. */" Fixed. > > Pedro> + /* Check if a record target is already running. */ > Pedro> + if (find_record_target ()) > > != NULL > Fixed. > Pedro> +/* Find the record_stratum target in the target stack. */ > Pedro> +extern struct target_ops *find_record_target (void); > > I think this should mention that it can return NULL. Fixed. Below's what I pushed after retesting. Thanks. ---------- Subject: [PATCH] Fix "is a record target open" checks. RECORD_IS_USED and record_full_open look at current_target.to_stratum to determine whether a record target is in use. This is wrong because arch_stratum is greater than record_stratum, so if an arch_stratum target is pushed, RECORD_IS_USED and record_full_open will miss it. To fix this, we can use the existing find_record_target instead, which looks up for a record stratum target across the target stack. Since that means exporting find_record_target in record.h, RECORD_IS_USED ends up redundant, so the patch eliminates it. That exercise then reveals other issues: - adjust_pc_after_break is gating record_full_... calls based on RECORD_IS_USED. But, record_full_ calls shouldn't be made when recording with the record-btrace target. So this adds a new record_full_is_used predicate to be used in that spot. - record_full_open says "Process record target already running", even if the recording target is record-btrace ("process record" is the original complete name of the record-full target). record_btrace_open only says "The process is already being recorded." and does not suggest "record stop", like record-full does. The patch factors out and merges that error to a new record_preopen function that all record targets call in their open routine. Tested on x86_64 Fedora 17. gdb/ 2014-01-14 Pedro Alves Tom Tromey * infrun.c (use_displaced_stepping): Use find_record_target instead of RECORD_IS_USED. (adjust_pc_after_break): Use record_full_is_used instead of RECORD_IS_USED. * record-btrace.c (record_btrace_open): Call record_preopen instead of checking RECORD_IS_USED. * record-full.c (record_full_shortname) (record_full_core_shortname): New globals. (record_full_is_used): New function. (find_full_open): Call record_preopen instead of checking RECORD_IS_USED. (init_record_full_ops): Set the target's shortname to record_full_shortname. (init_record_full_core_ops): Set the target's shortname to record_full_core_shortname. * record-full.h (record_full_is_used): Declare. * record.c (find_record_target): Make extern. (record_preopen): New function. * record.h (RECORD_IS_USED): Delete macro. (find_record_target, record_preopen): Declare functions. --- gdb/ChangeLog | 24 ++++++++++++++++++++++++ gdb/infrun.c | 4 ++-- gdb/record-btrace.c | 3 +-- gdb/record-full.c | 18 ++++++++++++++---- gdb/record-full.h | 4 ++++ gdb/record.c | 15 +++++++++++++-- gdb/record.h | 10 ++++++++-- 7 files changed, 66 insertions(+), 12 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 40e703f..3daf77f 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,27 @@ +2014-01-14 Pedro Alves + Tom Tromey + + * infrun.c (use_displaced_stepping): Use find_record_target + instead of RECORD_IS_USED. + (adjust_pc_after_break): Use record_full_is_used instead of + RECORD_IS_USED. + * record-btrace.c (record_btrace_open): Call record_preopen + instead of checking RECORD_IS_USED. + * record-full.c (record_full_shortname) + (record_full_core_shortname): New globals. + (record_full_is_used): New function. + (find_full_open): Call record_preopen instead of checking + RECORD_IS_USED. + (init_record_full_ops): Set the target's shortname to + record_full_shortname. + (init_record_full_core_ops): Set the target's shortname to + record_full_core_shortname. + * record-full.h (record_full_is_used): Declare. + * record.c (find_record_target): Make extern. + (record_preopen): New function. + * record.h (RECORD_IS_USED): Delete macro. + (find_record_target, record_preopen): Declare functions. + 2014-01-14 Yao Qi * gdbarch.sh (core_xfer_shared_libraries): Change its argument diff --git a/gdb/infrun.c b/gdb/infrun.c index 73038a3..311bf9c 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1240,7 +1240,7 @@ use_displaced_stepping (struct gdbarch *gdbarch) return (((can_use_displaced_stepping == AUTO_BOOLEAN_AUTO && non_stop) || can_use_displaced_stepping == AUTO_BOOLEAN_TRUE) && gdbarch_displaced_step_copy_insn_p (gdbarch) - && !RECORD_IS_USED); + && find_record_target () == NULL); } /* Clean out any stray displaced stepping state. */ @@ -3048,7 +3048,7 @@ adjust_pc_after_break (struct execution_control_state *ecs) { struct cleanup *old_cleanups = make_cleanup (null_cleanup, NULL); - if (RECORD_IS_USED) + if (record_full_is_used ()) record_full_gdb_operation_disable_set (); /* When using hardware single-step, a SIGTRAP is reported for both diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index 5fd26e2..c3330e9 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -142,8 +142,7 @@ record_btrace_open (char *args, int from_tty) DEBUG ("open"); - if (RECORD_IS_USED) - error (_("The process is already being recorded.")); + record_preopen (); if (!target_has_execution) error (_("The program is not being run.")); diff --git a/gdb/record-full.c b/gdb/record-full.c index a9fc018..a93418c 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -208,6 +208,19 @@ static ULONGEST record_full_insn_count; static struct target_ops record_full_ops; static struct target_ops record_full_core_ops; +/* See record-full.h. */ + +int +record_full_is_used (void) +{ + struct target_ops *t; + + t = find_record_target (); + return (t == &record_full_ops + || t == &record_full_core_ops); +} + + /* Command lists for "set/show record full". */ static struct cmd_list_element *set_record_full_cmdlist; static struct cmd_list_element *show_record_full_cmdlist; @@ -907,10 +920,7 @@ record_full_open (char *name, int from_tty) if (record_debug) fprintf_unfiltered (gdb_stdlog, "Process record: record_full_open\n"); - /* Check if record target is already running. */ - if (current_target.to_stratum == record_stratum) - error (_("Process record target already running. Use \"record stop\" to " - "stop record target first.")); + record_preopen (); /* Reset the tmp beneath pointers. */ tmp_to_resume_ops = NULL; diff --git a/gdb/record-full.h b/gdb/record-full.h index 517d786..ef3b387 100644 --- a/gdb/record-full.h +++ b/gdb/record-full.h @@ -25,6 +25,10 @@ extern int record_full_memory_query; extern int record_full_arch_list_add_reg (struct regcache *regcache, int num); extern int record_full_arch_list_add_mem (CORE_ADDR addr, int len); extern int record_full_arch_list_add_end (void); + +/* Returns true if the process record target is open. */ +extern int record_full_is_used (void); + extern struct cleanup *record_full_gdb_operation_disable_set (void); #endif /* RECORD_FULL_H */ diff --git a/gdb/record.c b/gdb/record.c index e0df6b1..f2cfcc8 100644 --- a/gdb/record.c +++ b/gdb/record.c @@ -57,9 +57,9 @@ struct cmd_list_element *info_record_cmdlist = NULL; if (record_debug) \ fprintf_unfiltered (gdb_stdlog, "record: " msg "\n", ##args) -/* Find the record target in the target stack. */ +/* See record.h. */ -static struct target_ops * +struct target_ops * find_record_target (void) { struct target_ops *t; @@ -88,6 +88,17 @@ require_record_target (void) /* See record.h. */ +void +record_preopen (void) +{ + /* Check if a record target is already running. */ + if (find_record_target () != NULL) + error (_("The process is already being recorded. Use \"record stop\" to " + "stop recording first.")); +} + +/* See record.h. */ + int record_read_memory (struct gdbarch *gdbarch, CORE_ADDR memaddr, gdb_byte *myaddr, diff --git a/gdb/record.h b/gdb/record.h index ab5ea4b..962e382 100644 --- a/gdb/record.h +++ b/gdb/record.h @@ -22,8 +22,6 @@ struct cmd_list_element; -#define RECORD_IS_USED (current_target.to_stratum == record_stratum) - extern unsigned int record_debug; /* Allow record targets to add their own sub-commands. */ @@ -63,4 +61,12 @@ extern void record_mourn_inferior (struct target_ops *); /* The default "to_kill" target method for record targets. */ extern void record_kill (struct target_ops *); +/* Find the record_stratum target in the current target stack. + Returns NULL if none is found. */ +extern struct target_ops *find_record_target (void); + +/* This is to be called by record_stratum targets' open routine before + it does anything. */ +extern void record_preopen (void); + #endif /* _RECORD_H_ */ -- 1.7.11.7