From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113434 invoked by alias); 30 Aug 2017 22:32:10 -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 113299 invoked by uid 89); 30 Aug 2017 22:32:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=routing 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; Wed, 30 Aug 2017 22:31:54 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CD76A356C8; Wed, 30 Aug 2017 22:31:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CD76A356C8 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=keiths@redhat.com Received: from valrhona.uglyboxes.com (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 734BE77705; Wed, 30 Aug 2017 22:31:52 +0000 (UTC) Subject: Re: [PATCH] Add a 'starti' command. To: John Baldwin , gdb-patches@sourceware.org References: <20170829225457.66096-1-jhb@FreeBSD.org> From: Keith Seitz Message-ID: <64d0abc2-4fb1-7624-9e13-2d28a6eb2bbf@redhat.com> Date: Wed, 30 Aug 2017 22:32:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170829225457.66096-1-jhb@FreeBSD.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00537.txt.bz2 Hi, I've looked over this patch, and I have a few small requests. [I am not a global maintainer and cannot officially approve your patch.] On 08/29/2017 03:54 PM, John Baldwin wrote: > This works like 'start' but it stops at the first instruction rather than > the first line in main(). This is useful if one wants to single step > through runtime linker startup. I like this! > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index bd9ead8a45..5e9173ff44 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -520,12 +520,14 @@ prepare_execution_command (struct target_ops *target, int background) > } > } > > +enum run_break { NONE, MAIN, FIRST_INSTR }; > + This does not conform to our coding standard. We typically require a comment before the enum explaining its usage/purpose and additional comments explaining what each member means. Something like: /* Determines how run_command_1 will behave. */ enum run_break { /* Run the inferior. */ NONE, /* Run the inferior, stopping at the first instruction of main. */ MAIN, // ... }; While I would rather see these options be a little more succinctly named, such as RUN_NONE, RUN_STOP_MAIN, RUN_STOP_MAIN_FIRST_INSN, I'm not going to ask for that change. But please do consider it. > /* Implement the "run" command. If TBREAK_AT_MAIN is set, then insert > a temporary breakpoint at the begining of the main program before > running the program. */ > > static void > -run_command_1 (char *args, int from_tty, int tbreak_at_main) > +run_command_1 (char *args, int from_tty, enum run_break run_break) The comment to this function needs updating: it still refers to the (removed) argument TBREAK_AT_MAIN and does not mention RUN_BREAK. > { > const char *exec_file; > struct cleanup *old_chain; > @@ -632,6 +634,14 @@ run_command_1 (char *args, int from_tty, int tbreak_at_main) > has done its thing; now we are setting up the running program. */ > post_create_inferior (¤t_target, 0); > > + if (run_break == FIRST_INSTR) > + { > + gdb::unique_xmalloc_ptr loc > + (xstrdup (("*" + std::to_string (regcache_read_pc > + (get_current_regcache ()))).c_str ())); > + tbreak_command (loc.get (), 0); > + } > + I understand why you did it this way, but I really think using create_breakpoint directly is a better option instead of routing through an existing CLI command (tbreak_command). Something along the lines of: CORE_ADDR insn = regcache_read_pc (get_current_regcache ()); event_location_up loc = new_address_location (insn, NULL, 0); create_breakpoint (get_current_arch (), loc.get (), NULL /* cond_string */, -1 /* thread */, NULL /* extra_string */, 0 /* parse_extra */, 1 /* tempflag */, bp_breakpoint, 0 /* ignore_count */, AUTO_BOOLEAN_FALSE /* pending_break_support */, &bkpt_breakpoint_ops, from_tty, 1 /* enabled */, 0 /* internal */, 0 /* flags */); > /* Start the target running. Do not use -1 continuation as it would skip > breakpoint right at the entry point. */ > proceed (regcache_read_pc (get_current_regcache ()), GDB_SIGNAL_0); > @@ -660,7 +670,17 @@ start_command (char *args, int from_tty) > error (_("No symbol table loaded. Use the \"file\" command.")); > > /* Run the program until reaching the main procedure... */ > - run_command_1 (args, from_tty, 1); > + run_command_1 (args, from_tty, MAIN); > +} > + > +/* Start the execution of the program until the first instruction. */ > + > +static void > +starti_command (char *args, int from_tty) > +{ > + > + /* Run the program until reaching the first instruction... */ > + run_command_1 (args, from_tty, FIRST_INSTR); > } I'm all for commenting code, but the comment describing the function is sufficient. :-) > static int > @@ -3415,6 +3435,12 @@ You may specify arguments to give to your program, just as with the\n\ > \"run\" command.")); > set_cmd_completer (c, filename_completer); > > + c = add_com ("starti", class_run, starti_command, _("\ > +Start the debugged program stopping at the first instruction.\n\ > +You may specify arguments to give to your program, just as with the\n\ > +\"run\" command.")); > + set_cmd_completer (c, filename_completer); > + > add_com ("interrupt", class_run, interrupt_command, > _("Interrupt the execution of the debugged program.\n\ > If non-stop mode is enabled, interrupt only the current thread,\n\ > I would take the common start/starti text and put it into a macro similar to the way the breakpoint commands are setup. Alas the last thing missing is a test case for this. It doesn't have to be anything fancy, just check that with no breakpoints installed, starti will actually cause the inferior to stop somewhere/anywhere. [I don't think we can generically do much better than this, e.g., testing if we actually do stop at the first insn.] Keith