From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x344.google.com (mail-wm1-x344.google.com [IPv6:2a00:1450:4864:20::344]) by sourceware.org (Postfix) with ESMTPS id 37A0D38708A2 for ; Tue, 22 Sep 2020 13:52:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 37A0D38708A2 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x344.google.com with SMTP id y15so3448408wmi.0 for ; Tue, 22 Sep 2020 06:52:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=9TOyuYkGAN4j7xwpimsuiph/7fNnXxfkIvuGwci0/LY=; b=eMrygb7tkpkVgAnV8TNCCAvLylfvxWyGTQS4LlDd+by+ioiWfkoPyJrebc68x6qfSQ DW9DumNTybZOt6DmQ622LF1+Tx+yS4EYtJl5RYBLM7zalbZrvrez9V+MiF2PB6MoZVAJ tYuqw+TWahRMdhj6mhZ/USZC2LxDKZmF4lbiVypQB7+Uiprx4t3VtDgV5G7rl7EJK/U+ XkiNzOap7U8Hi5/XEQb60+0T+0PkKfl+hSs8vu4fwOfJQfnFkSGaOARQ/Zv2x4eQUjtR F6N2uI1AERU3ZA+QwHF0u3JTH4q2DIQmtwh46+3Xo/dxJeht3bT/SgoPZb6TGm5h4ozX odSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=9TOyuYkGAN4j7xwpimsuiph/7fNnXxfkIvuGwci0/LY=; b=VncIvnJl6+h1sbx91I9gyW/Pl+ab4+DOHHAnoVsLZAWjJZ7rS8PWoWJGRjBLqe6Gov XXEETXUcdflf0GerFIPceui35A8Ug/fyBZCxHB6HtNX+Qdi1SU6O0OnMzIttDVN46a/P xcDnasBOwJFb6ebz1vZ8vpTqEO3Np4y3D3LJCu9dvB0fRsiMDT7tq2aYkhYpGOr5oz2p 4tvlF6O1b6QWCzFfWr7v5TkPkFzLSJfFUE3cgB4fr9UeSapkO7Frqkm5J7dleG+j510F fZjhALcLWnVuUumZjY1Ms8UIXVui7VsNKCsfqqJVIzqzkpFFbqNhW1ifOLoKtLYFiyOq HIIQ== X-Gm-Message-State: AOAM531xf9A/qQcYvJF5eeh9S9Tv9CAHjyqY4QVoTKcHLJ0z79pGcqdd UdNGwotE+UFnKN4qs09IvI6TCw== X-Google-Smtp-Source: ABdhPJysADuWZGbYv3CxZHqJDXmdEZzrLRIlrlYf0zFQKqHBw5plmPm9xh5waw2WwrB3EMKeNZmUMQ== X-Received: by 2002:a05:600c:210c:: with SMTP id u12mr7901wml.185.1600782720074; Tue, 22 Sep 2020 06:52:00 -0700 (PDT) Received: from localhost (host31-53-80-104.range31-53.btcentralplus.com. [31.53.80.104]) by smtp.gmail.com with ESMTPSA id v17sm28140208wrc.23.2020.09.22.06.51.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 22 Sep 2020 06:51:58 -0700 (PDT) Date: Tue, 22 Sep 2020 14:51:57 +0100 From: Andrew Burgess To: Gareth Rees Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: Fix from_tty argument to gdb.execute in Python. Message-ID: <20200922135157.GH1540618@embecosm.com> References: <20200922102343.8440-1-grees@undo.io> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200922102343.8440-1-grees@undo.io> X-Operating-System: Linux/5.8.9-101.fc31.x86_64 (x86_64) X-Uptime: 14:39:48 up 3 days, 21:47, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Sep 2020 13:52:02 -0000 * Gareth Rees [2020-09-22 11:23:43 +0100]: > Prior to commit 56bcdbea2b, the from_tty keyword argument to the > Python function gdb.execute controlled whether the command took input > from the terminal. When from_tty=True, the "starti" command prompted > the user: > > (gdb) python gdb.execute("starti", from_tty=True) > The program being debugged has been started already. > Start it from the beginning? (y or n) y > Starting program: /bin/true > > Program stopped. > > When from_tty=False, it did not prompt the user, and "yes" was assumed: > > (gdb) python gdb.execute('starti', from_tty=False) > > Program stopped. > > However, after commit 56bcdbea2b, the from_tty keyword argument no > longer had this effect. For example, as of commit 7ade7fba75: > > (gdb) python gdb.execute('starti', from_tty=True) > The program being debugged has been started already. > Start it from the beginning? (y or n) [answered Y; input not from terminal] > Starting program: /bin/true > > Program stopped. > > Note the "[answered Y; input not from terminal]" in the output even > though from_tty=True was requested. > > Looking at commit 56bcdbea2b, it seems that the behaviour of the > from_tty argument was changed accidentally. The commit message said: > > Let gdb.execute handle multi-line commands > > This changes the Python API so that gdb.execute can now handle > multi-line commands, like "commands" or "define". > > and there was no mention of changing the effect of the from_tty > argument. It looks as though the code for setting the instream to 0 > was accidentally moved from execute_user_command() to > execute_control_commands() along with the code for iterating over a > series of command lines. > > Accordingly, the simplest way to fix this is to partially reverse > commit 56bcdbea2b by moving the code for setting the instream to 0 > back to execute_user_command() where it was to begin with. > > Additionally, add a test case to reduce the risk of similar breakage > in future. > > gdb/ChangeLog: > > * cli/cli-script.c (execute_control_commands): don't set > instream to 0 here as this breaks the from_tty argument to > gdb.execute in Python. > (execute_user_command): set instream to 0 here instead. > > gdb/testsuite/ChangeLog: > > * gdb.python/python.exp: add test cases for the from_tty > argument to gdb.execute. > --- > gdb/cli/cli-script.c | 16 ++++++++-------- > gdb/testsuite/gdb.python/python.exp | 13 +++++++++++++ > 2 files changed, 21 insertions(+), 8 deletions(-) > > diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c > index da4a41023a..4adcda85e6 100644 > --- a/gdb/cli/cli-script.c > +++ b/gdb/cli/cli-script.c > @@ -392,14 +392,6 @@ execute_cmd_post_hook (struct cmd_list_element *c) > void > execute_control_commands (struct command_line *cmdlines, int from_tty) > { > - /* Set the instream to 0, indicating execution of a > - user-defined function. */ > - scoped_restore restore_instream > - = make_scoped_restore (¤t_ui->instream, nullptr); > - scoped_restore save_async = make_scoped_restore (¤t_ui->async, 0); > - scoped_restore save_nesting > - = make_scoped_restore (&command_nest_depth, command_nest_depth + 1); > - > while (cmdlines) > { > enum command_control_type ret = execute_control_command (cmdlines, > @@ -464,6 +456,14 @@ execute_user_command (struct cmd_list_element *c, const char *args) > if (user_args_stack.size () > max_user_call_depth) > error (_("Max user call depth exceeded -- command aborted.")); > > + /* Set the instream to 0, indicating execution of a > + user-defined function. */ > + scoped_restore restore_instream > + = make_scoped_restore (¤t_ui->instream, nullptr); > + scoped_restore save_async = make_scoped_restore (¤t_ui->async, 0); > + scoped_restore save_nesting > + = make_scoped_restore (&command_nest_depth, command_nest_depth + 1); I'm happy with the change to instream being moved, however, I'm not sure about the other two lines. command_nest_depth especially I think should be left where it is now, with current HEAD my gdb session looks like this: .... (gdb) set trace-commands on (gdb) python gdb.execute ("starti", False) +python gdb.execute ("starti", False) ++starti Program stopped. And with your patch: ... (gdb) set trace-commands on (gdb) python gdb.execute ("starti", False) +python gdb.execute ("starti", False) +starti Program stopped. I think the version in current HEAD is better. I'm tempted to think the async change should also remain in its current location. I believe this means that if GDB is in async mode, a Python gdb.execute of something like next/continue will operate in synchronous mode before returning. Though I don't know if that would mean there's no way to initiate async control from Python.... ... I think you might want to see what others have to say on this. Thanks, Andrew > + > execute_control_commands (cmdlines, 0); > } > > diff --git a/gdb/testsuite/gdb.python/python.exp b/gdb/testsuite/gdb.python/python.exp > index a031ea5a18..017f33afe5 100644 > --- a/gdb/testsuite/gdb.python/python.exp > +++ b/gdb/testsuite/gdb.python/python.exp > @@ -526,3 +526,16 @@ gdb_test "print \$cvar3" "= void" \ > # Test PR 23669, the following would invoke the "commands" command instead of > # "show commands". > gdb_test "python gdb.execute(\"show commands\")" "$decimal print \\\$cvar3.*" > + > +# Test that the from_tty argument to gdb.execute is effective. If > +# False, the user is not prompted for decisions such as restarting the > +# program, and "yes" is assumed. If True, the user is prompted. > +gdb_test "python gdb.execute('starti', from_tty=False)" \ > + "Program stopped.*" \ > + "starti via gdb.execute, not from tty" > +gdb_test_multiple "python gdb.execute('starti', from_tty=True)" \ > + "starti via gdb.execute, from tty" { > + -re {The program being debugged has been started already\.\r\nStart it from the beginning\? \(y or n\) $} { > + gdb_test "y" "Starting program:.*" "starti via interactive input" > + } > +} > -- > 2.26.0 >