From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 55365 invoked by alias); 10 Sep 2015 12:11:51 -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 55352 invoked by uid 89); 10 Sep 2015 12:11:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00,MIME_BASE64_BLANKS,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: na01-bn1-obe.outbound.protection.outlook.com Received: from mail-bn1bn0101.outbound.protection.outlook.com (HELO na01-bn1-obe.outbound.protection.outlook.com) (157.56.110.101) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA256 encrypted) ESMTPS; Thu, 10 Sep 2015 12:11:50 +0000 Received: from BY2PR0301MB1527.namprd03.prod.outlook.com (10.163.27.153) by BY2PR0301MB1527.namprd03.prod.outlook.com (10.163.27.153) with Microsoft SMTP Server (TLS) id 15.1.262.15; Thu, 10 Sep 2015 12:11:47 +0000 Received: from BY2PR0301MB1527.namprd03.prod.outlook.com ([10.163.27.153]) by BY2PR0301MB1527.namprd03.prod.outlook.com ([10.163.27.153]) with mapi id 15.01.0262.011; Thu, 10 Sep 2015 12:11:47 +0000 From: Nistor Mihail To: Yao Qi CC: "gdb-patches@sourceware.org" Subject: RE: [PATCH] gdb/18947: [aarch64]Step into shared library is very slow. Date: Thu, 10 Sep 2015 12:11:00 -0000 Message-ID: References: <1441876920-32009-1-git-send-email-mihail.nistor@freescale.com> <86613i7b1b.fsf@gmail.com> In-Reply-To: <86613i7b1b.fsf@gmail.com> authentication-results: spf=none (sender IP is ) smtp.mailfrom=mihail.nistor@freescale.com; x-microsoft-exchange-diagnostics: 1;BY2PR0301MB1527;5:7Y1jVx0P7ycAdS2qQjnOx5lL7butRreLQPe6ShhO+2au64yyy9DB4snp+JrCaApj03ENTpLm8bfnap8xFbN+4R7kEonPcpf4Vry+DPwCDBkOOuwlilKCIkUgFm1UX7MD/mW0mlHmfN5UB8SrtOk2Ag==;24:Gajnr5vEnafIarVVYpbwcLGf6K6L8YZpaaCPuZeUVE6tqjZyLazsqY92NioZgZ83fSAjkW7coJvT8acwLz6WtIpCq/4JB1Ic8E67uudI3C8=;20:GmW1kfHFuxWhoAwZ19wsVBQX7UlUUxEXL+UbiumD0NTfrhqEp76rLdakDixg+xm8Iq8ClFpFBQyrwvbLHKohTg== x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BY2PR0301MB1527; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(8121501046)(5005006)(3002001);SRVR:BY2PR0301MB1527;BCL:0;PCL:0;RULEID:;SRVR:BY2PR0301MB1527; x-forefront-prvs: 06952FC175 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(189002)(199003)(377454003)(377424004)(54534003)(13464003)(77096005)(2950100001)(5007970100001)(5001960100002)(5002640100001)(86362001)(33656002)(54356999)(106356001)(1411001)(11100500001)(76176999)(19580395003)(4001540100001)(5001830100001)(5001860100001)(19580405001)(50986999)(101416001)(87936001)(77156002)(97736004)(10400500002)(62966003)(105586002)(64706001)(99286002)(81156007)(40100003)(189998001)(66066001)(74316001)(92566002)(5003600100002)(76576001)(2900100001)(46102003)(122556002)(102836002)(110136002)(106116001)(5004730100002)(68736005);DIR:OUT;SFP:1102;SCL:1;SRVR:BY2PR0301MB1527;H:BY2PR0301MB1527.namprd03.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; received-spf: None (protection.outlook.com: freescale.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:23 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 MIME-Version: 1.0 X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Sep 2015 12:11:47.1212 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY2PR0301MB1527 X-SW-Source: 2015-09/txt/msg00159.txt.bz2 SGkgWWFvLA0KDQpJIGhhdmUgcnVuIEdEQiB0ZXN0c3VpdGUgZm9yIGFhcmNo NjQgYW5kIEkgZG9uJ3Qgc2VlIGFueSByZWdyZXNzaW9uLiANCg0KQmVzdCBy ZWdhcmRzLA0KTWloYWkNCg0KLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0N CkZyb206IFlhbyBRaSBbbWFpbHRvOnFpeWFvbHRjQGdtYWlsLmNvbV0gDQpT ZW50OiBUaHVyc2RheSwgU2VwdGVtYmVyIDEwLCAyMDE1IDI6NDYgUE0NClRv OiBOaXN0b3IgTWloYWlsLU1OSVNUT1IxIDxtaWhhaWwubmlzdG9yQGZyZWVz Y2FsZS5jb20+DQpDYzogZ2RiLXBhdGNoZXNAc291cmNld2FyZS5vcmcNClN1 YmplY3Q6IFJlOiBbUEFUQ0hdIGdkYi8xODk0NzogW2FhcmNoNjRdU3RlcCBp bnRvIHNoYXJlZCBsaWJyYXJ5IGlzIHZlcnkgc2xvdy4NCg0KTWloYWlsLU1h cmlhbiBOaXN0b3IgPG1paGFpbC5uaXN0b3JAZnJlZXNjYWxlLmNvbT4gd3Jp dGVzOg0KDQpIaSBNaWhhaWwtTWFyaWFuLA0KDQo+IEluc3RhbGwgZ2RiYXJj aF9za2lwX3NvbGliX3Jlc29sdmVyIG9uIGFhcmNoNjQgR05VL0xpbnV4DQo+ DQo+IGdkYi9DaGFuZ2VMb2c6DQo+IDIwMTUtMDktMTAgIE1paGFpbC1NYXJp YW4gTmlzdG9yICA8bWloYWlsLm5pc3RvckBmcmVlc2NhbGUuY29tPg0KPg0K PiAJUFIgZ2RiLzE4OTQ3DQo+IAkqIGFhcmNoNjQtbGludXgtdGRlcC5jOiAo YWFyY2g2NF9saW51eF9pbml0X2FiaSk6IEluc3RhbGwNCj4gCWdsaWJjX3Nr aXBfc29saWJfcmVzb2x2ZXIgYXMgZ2RiYXJjaF9za2lwX3NvbGliX3Jlc29s dmVyIGNhbGxiYWNrLg0KDQpUaGUgcGF0Y2ggaXMgT0sgaWYgeW91IHJ1biBH REIgdGVzdHN1aXRlIGZvciBhYXJjaDY0LWxpbnV4LCBhbmQgbm8gcmVncmVz c2lvbnMuICBJZiB5b3UgZG9uJ3QgaGF2ZSBhbiBlbnYgc2V0IHVwIGZvciBy dW5uaW5nIEdEQiB0ZXN0c3VpdGUsIEkgY2FuIHRlc3QgaXQgZm9yIHlvdS4N Cg0KLS0NCllhbyAo6b2Q5bCnKQ0K >From gdb-patches-return-125920-listarch-gdb-patches=sources.redhat.com@sourceware.org Thu Sep 10 12:43:44 2015 Return-Path: Delivered-To: listarch-gdb-patches@sources.redhat.com Received: (qmail 41179 invoked by alias); 10 Sep 2015 12:43:44 -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 Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 41167 invoked by uid 89); 10 Sep 2015 12:43:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 10 Sep 2015 12:43:41 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 87D5080091; Thu, 10 Sep 2015 12:43:40 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t8AChcuV002708; Thu, 10 Sep 2015 08:43:39 -0400 Message-ID: <55F17AFA.5080102@redhat.com> Date: Thu, 10 Sep 2015 12:43:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Don Breazeal , gdb-patches@sourceware.org Subject: Re: [PATCH v3 1/4] Extended-remote follow exec References: <1438298360-29594-1-git-send-email-donb@codesourcery.com> <1441839937-22251-1-git-send-email-donb@codesourcery.com> <1441839937-22251-2-git-send-email-donb@codesourcery.com> In-Reply-To: <1441839937-22251-2-git-send-email-donb@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-09/txt/msg00160.txt.bz2 Content-length: 11167 On 09/10/2015 12:05 AM, Don Breazeal wrote: > Hi Pedro, > > This is an updated version of the patch previously submitted here: > https://sourceware.org/ml/gdb-patches/2015-07/msg00924.html. Changes > from the previous version include: > > * In gdbserver, when an exec event occurs, gdbserver deletes all > of the data (inferior, lwps, threads) associated with the execing > process and replaces it with a new set of data. > > * In GDB, the remote exec-file is now stored per-inferior in the > inferior's program space as a REGISTRY field. > > * In GDB, a new target hook, target_follow_exec, is used to enable > storing the remote exec-file as per-inferior data. > > * In GDB, follow_exec now calls add_inferior_with_spaces for mode > "new" in place of add_inferior and the calls to set up the program > and address spaces. > > Some of the things that were part of the previous patchset were > eliminated as a result of these changes, including: > > * Deleting "vanished" lwps in gdbserver/linux-low.c:send_sigstop. > > * Fiddling with the regcache and r_debug in > gdbserver/linux-low.c:handle_extended_wait. > > * Fiddling with the inferior's architecture in > remote.c:remote_parse_stop_reply. > > A couple of your questions about the previous version of the patch still > apply, in spite of the rework. Regarding the handling of the exec event > in linux-low.c:handle_extended_wait: > >>> + /* Mark the exec status as pending. */ >>> + event_lwp->stopped = 1; >>> + event_lwp->status_pending_p = 1; >>> + event_lwp->status_pending = wstat; >>> + event_thr->last_resume_kind = resume_stop; >> >> Shouldn't this be resume_continue? > > My thinking here is that as far as gdbserver is concerned, we *do* want > to use resume_stop, so that we stop and report the event to GDB. It will > be up to GDB whether to continue from this point. Does that make sense? Not really -- putting exec events out of the picture, consider: If you simply continue a thread (vCont;c) and it hits a breakpoint, it'll have last_resume_kind==resume_continue, and we still report the event to gdb, and it's still up to GDB whether to continue past the breakpoint. So if you set last_resume_kind to resume_continue, and drop this hunk: > @@ -3373,7 +3463,8 @@ linux_wait_1 (ptid_t ptid, > ourstatus->value.sig = GDB_SIGNAL_0; > } > else if (current_thread->last_resume_kind == resume_stop > - && WSTOPSIG (w) != SIGSTOP) > + && WSTOPSIG (w) != SIGSTOP > + && ourstatus->kind != TARGET_WAITKIND_EXECD) > { > /* A thread that has been requested to stop by GDB with vCont;t, > but, it stopped for other reasons. */ > @@ -5801,6 +5892,14 @@ linux_supports_vfork_events (void) > return linux_supports_tracefork (); > } ... what doesn't work? > @@ -571,6 +598,50 @@ handle_extended_wait (struct lwp_info *event_lwp, int wstat) > /* Report the event. */ > return 0; > } > + else if (event == PTRACE_EVENT_EXEC && report_exec_events) > + { > + struct process_info *proc; > + ptid_t event_ptid; > + pid_t event_pid; > + > + if (debug_threads) > + { > + debug_printf ("HEW: Got exec event from LWP %ld\n", > + lwpid_of (event_thr)); > + } > + > + /* Get the event ptid. */ > + event_ptid = event_thr->entry.id; event_ptid = ptid_of (event_thr); > + event_pid = ptid_get_pid (event_ptid); > + > + /* Delete the execing process and all its threads. */ > + proc = get_thread_process (event_thr); > + linux_mourn (proc); > + current_thread = NULL; > + > + /* Create a new process/lwp/thread. */ > + proc = linux_add_process (event_pid, 0); > + event_lwp = add_lwp (event_ptid); > + event_thr = get_lwp_thread (event_lwp); > + gdb_assert (current_thread == event_thr); > + linux_arch_setup_thread (event_thr); > + > + /*set the event status*/ Uppercase, period at end, double space. > + event_lwp->waitstatus.kind = TARGET_WAITKIND_EXECD; > + event_lwp->waitstatus.value.execd_pathname > + = xstrdup (linux_proc_pid_to_exec_file (lwpid_of (event_thr))); > + > + /* Mark the exec status as pending. */ > + event_lwp->stopped = 1; > + event_lwp->status_pending_p = 1; > + event_lwp->status_pending = wstat; > + event_thr->last_resume_kind = resume_stop; > + event_thr->last_status.kind = TARGET_WAITKIND_IGNORE; > + > + /* Report the event. */ > + *orig_event_lwp = event_lwp; > + return 0; > + } > > internal_error (__FILE__, __LINE__, _("unknown ptrace event %d"), event); > } > @@ -1134,6 +1135,25 @@ prepare_resume_reply (char *buf, ptid_t ptid, > buf = write_ptid (buf, status->value.related_pid); > strcat (buf, ";"); > } > + else if (status->kind == TARGET_WAITKIND_EXECD && multi_process) > + { > + enum gdb_signal signal = GDB_SIGNAL_TRAP; > + const char *event = "exec"; > + char hexified_pathname[PATH_MAX*2]; Spaces around *. > + > + sprintf (buf, "T%02x%s:", signal, event); > + buf += strlen (buf); > + > + /* Encode pathname to hexified format. */ > + bin2hex ((const gdb_byte *) status->value.execd_pathname, > + hexified_pathname, > + strlen (status->value.execd_pathname)); > + > + sprintf (buf, "%s;", hexified_pathname); > + xfree (status->value.execd_pathname); > + status->value.execd_pathname = NULL; > + buf += strlen (buf); > + } > else > sprintf (buf, "T%02x", status->value.sig); > > @@ -619,6 +622,62 @@ get_remote_state (void) > return get_remote_state_raw (); > } > > +/* Cleanup routine for the remote module's pspace data. */ > + > +static void > +remote_pspace_data_cleanup (struct program_space *pspace, void *arg) > +{ > + char *remote_exec_file = arg; > + > + if (remote_exec_file != NULL) > + xfree (remote_exec_file); No need to check for NULL before calling xfree. > +} > + > +/* Fetch the remote exec-file from the current program space. */ > + > +static char * Can this be const char * ? > +get_remote_exec_file (void) > +{ > + char *remote_exec_file; > + > + remote_exec_file = program_space_data (current_program_space, > + remote_pspace_data); How about adding: if (remote_exec_file == NULL) return ""; > + return remote_exec_file; avoiding callers having to do it. > +} > + > +/* Set the remote exec file for the current program space. */ > + > +static void > +set_remote_exec_file_1 (char *remote_exec_file) > +{ > + char *old_file = get_remote_exec_file (); > + Here's you'd use old_file = program_space_data (current_program_space, remote_pspace_data); directly. It would seem super fine to me given the set_program_space_data just below. > + if (old_file != NULL) > + xfree (old_file); No need for NULL check. > + > + set_program_space_data (current_program_space, remote_pspace_data, > + xstrdup (remote_exec_file)); > +} > + > +/* The "set/show remote exec-file" set hook. */ > + > +static void > +set_remote_exec_file (char *ignored, int from_tty, > + struct cmd_list_element *c) > +{ > + gdb_assert (*(char **) c->var != NULL); > + set_remote_exec_file_1 (*(char **) c->var); Use temp var please. E.g.: char *file = *(char **) c->var; gdb_assert (file != NULL); set_remote_exec_file_1 (file); Or see another suggestion below. > +} > + > +/* The "set/show remote exec-file" show hook. */ > + > +static void > +show_remote_exec_file (struct ui_file *file, int from_tty, > + struct cmd_list_element *cmd, const char *value) > +{ > + fprintf_filtered (file, "%s\n", get_remote_exec_file ()); > +} > + > static int > compare_pnums (const void *lhs_, const void *rhs_) > { > @@ -4779,6 +4842,28 @@ remote_follow_fork (struct target_ops *ops, int follow_child, > return 0; > } > > +/* Target follow-exec function for remote targets. Save EXECD_PATHNAME > + in the program space of the new inferior. On entry and at return the > + current inferior is the exec'ing inferior. INF is the new exec'd > + inferior, which may be the same as the exec'ing inferior unless > + follow-exec-mode is "new". */ > + > +static void > +remote_follow_exec (struct target_ops *ops, > + struct inferior *inf, char *execd_pathname) > +{ > + struct cleanup *old_chain = save_current_program_space (); > + > + /* We know that this is a target file name, so if it has the "target:" > + prefix we strip it off before saving it in the program space. */ > + if (is_target_filename (execd_pathname)) > + execd_pathname += strlen (TARGET_SYSROOT_PREFIX); > + > + set_current_program_space (inf->pspace); Why not pass down the pspace as parameter to set_remote_exec_file_1 etc., avoiding this? > + set_remote_exec_file_1 (execd_pathname); > + do_cleanups (old_chain); > +} > + > /* Same as remote_detach, but don't send the "D" packet; just disconnect. */ > > static void > @@ -5977,6 +6062,7 @@ remote_parse_stop_reply (char *buf, struct stop_reply *event) > struct remote_arch_state *rsa = get_remote_arch_state (); > ULONGEST addr; > char *p; > + int skipregs = 0; > > event->ptid = null_ptid; > event->rs = get_remote_state (); > @@ -13340,12 +13479,21 @@ Transfer files to and from the remote target system."), > _("Delete a remote file."), > &remote_cmdlist); > > - remote_exec_file = xstrdup (""); > - add_setshow_string_noescape_cmd ("exec-file", class_files, > - &remote_exec_file, _("\ > + { > + /* Pass a NULL (by reference) as the 'var' argument, since we do > + not have a single variable in which to store the value. The > + value is set per-inferior, stored in the program space. */ > + char *nullptr = NULL; Passing the address of a local variable can't be good. In: > +static void > +set_remote_exec_file (char *ignored, int from_tty, > + struct cmd_list_element *c) > +{ > + gdb_assert (*(char **) c->var != NULL); > + set_remote_exec_file_1 (*(char **) c->var); c->var points to nullptr. So this set command ends up poking memory to something random on the stack. I'd prefer leaving the old command variable global, but rename it remote_exec_file_var, like: /* The variable registered as control variable the set/show remote exec-file commands. Necessary because ... */ static char *remote_exec_file_var = ""; There's some precedent for that, in e.g., record_insn_history_size_setshow_var and history_size_setshow_var. Note that that way, referencing remote_exec_file_var directly in set_remote_exec_file avoids the casts. > + > + add_setshow_string_noescape_cmd ("exec-file", class_files, > + &nullptr, _("\ > Set the remote pathname for \"run\""), _("\ > -Show the remote pathname for \"run\""), NULL, NULL, NULL, > - &remote_set_cmdlist, &remote_show_cmdlist); > +Show the remote pathname for \"run\""), NULL, > + set_remote_exec_file, > + show_remote_exec_file, > + &remote_set_cmdlist, > + &remote_show_cmdlist); > + } Thanks, Pedro Alves