From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8509 invoked by alias); 29 Jun 2017 19:48:54 -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 7921 invoked by uid 89); 29 Jun 2017 19:48:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= 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; Thu, 29 Jun 2017 19:48:20 +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 DCDEF31D8C8; Thu, 29 Jun 2017 19:48:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com DCDEF31D8C8 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com DCDEF31D8C8 Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A458E17A9E; Thu, 29 Jun 2017 19:48:09 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches , Jerome Guitton Subject: Re: [PATCH] PR cli/21688: Fix multi-line/inline command differentiation References: <20170629020527.468-1-sergiodj@redhat.com> <90d0a1563dea6893b5dbcd8df19d0285@polymtl.ca> Date: Thu, 29 Jun 2017 19:48:00 -0000 In-Reply-To: <90d0a1563dea6893b5dbcd8df19d0285@polymtl.ca> (Simon Marchi's message of "Thu, 29 Jun 2017 21:08:35 +0200") Message-ID: <87d19mmv7q.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00800.txt.bz2 On Thursday, June 29 2017, Simon Marchi wrote: > On 2017-06-29 04:05, Sergio Durigan Junior wrote: >> This bug is a regression caused by the following commit: >> >> 604c4576fdcfc4e7c28f569b3748a1b6b4e0dbd4 is the first bad commit >> commit 604c4576fdcfc4e7c28f569b3748a1b6b4e0dbd4 >> Author: Jerome Guitton >> Date: Tue Jan 10 15:15:53 2017 +0100 >> >> The problem happens because, on cli/cli-script.c:process_next_line, >> GDB is not using the command line string to identify which command to >> run, but it instead using the 'struct cmd_list_element *' that is >> obtained by using the mentioned string. The problem with that is that >> the 'struct cmd_list_element *' doesn't have any information on >> whether the command issued by the user is a multi-line or inline one. >> >> A multi-line command is a command that will necessarily be composed of >> more than 1 line. For example: >> >> (gdb) if 1 >> >python >> >print ('hello') >> >end >> >end >> >> As can be seen in the example above, the 'python' command actually >> "opens" a new command line (represented by the change in the >> indentation) that will then be used to enter Python code. OTOH, an >> inline command is a command that is "self-contained" in a single line, >> for example: >> >> (gdb) if 1 >> >python print ('hello') >> >end >> >> This Python command is a one-liner, and therefore there is no other >> Python code that can be entered for this same block. There is also no >> change in the indentation. >> >> So, the fix is somewhat simple: we have to revert the change and use >> the full command line string passed to process_next_line in order to >> identify whether we're dealing with a multi-line or an inline command. >> This commit does just that. As can be seen, this regression also >> affects other languages, like guile or the compile framework. To make >> things clearer, I decided to create a new helper function responsible >> for identifying a non-inline command. >> >> Testcase is attached. > > Hi Sergio, > > Thanks for the fix and the test! I have a few comments below. Hey Simon, Thanks for the review! >> gdb/ChangeLog: >> 2017-06-29 Sergio Durigan Junior >> >> PR cli/21688 >> * cli/cli-script.c (command_name_equals_not_inline): New function. >> (process_next_line): Adjust 'if' clauses for "python", "compile" >> and "guile" to use command_name_equals_not_inline. >> >> gdb/testsuite/ChangeLog: >> 2017-06-29 Sergio Durigan Junior >> >> PR cli/21688 >> * gdb.python/py-cmd.exp (test_pr21688): New procedure. Call it. >> --- >> gdb/ChangeLog | 7 +++++++ >> gdb/cli/cli-script.c | 21 +++++++++++++++++---- >> gdb/testsuite/ChangeLog | 5 +++++ >> gdb/testsuite/gdb.python/py-cmd.exp | 30 >> ++++++++++++++++++++++++++++++ >> 4 files changed, 59 insertions(+), 4 deletions(-) >> >> diff --git a/gdb/ChangeLog b/gdb/ChangeLog >> index a82026f..194cda8 100644 >> --- a/gdb/ChangeLog >> +++ b/gdb/ChangeLog >> @@ -1,3 +1,10 @@ >> +2017-06-29 Sergio Durigan Junior >> + >> + PR cli/21688 >> + * cli/cli-script.c (command_name_equals_not_inline): New function. >> + (process_next_line): Adjust 'if' clauses for "python", "compile" >> + and "guile" to use command_name_equals_not_inline. >> + >> 2017-06-28 Pedro Alves >> >> * command.h: Include "common/scoped_restore.h". >> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c >> index e0e27ef..72f316f 100644 >> --- a/gdb/cli/cli-script.c >> +++ b/gdb/cli/cli-script.c >> @@ -900,6 +900,20 @@ command_name_equals (struct cmd_list_element >> *cmd, const char *name) >> && strcmp (cmd->name, name) == 0); >> } >> >> +/* Return true if NAME is the only command between COMMAND_START and >> + COMMAND_END. This is useful when we want to know whether the >> + command is inline (i.e., has arguments like 'python command1') or >> + is the start of a multi-line command block. */ >> + >> +static bool >> +command_name_equals_not_inline (const char *command_start, >> + const char *command_end, >> + const char *name) >> +{ >> + return (command_end - command_start == strlen (name) >> + && startswith (command_start, name)); >> +} >> + >> /* Given an input line P, skip the command and return a pointer to the >> first argument. */ >> >> @@ -997,21 +1011,20 @@ process_next_line (char *p, struct command_line >> **command, int parse_commands, >> { >> *command = build_command_line (commands_control, >> line_first_arg (p)); >> } >> - else if (command_name_equals (cmd, "python")) >> + else if (command_name_equals_not_inline (p_start, p_end, >> "python")) > > Another (maybe simpler) way would be to check > > else if (command_name_equals (cmd, "python") && *cmd_name == '\0') > > It's not clear when expressed like this though because cmd_name is not > well named at this point (it points just after the command name). Hm, right. Would you prefer this way instead? I don't have a strong opinion on this. > >> { >> /* Note that we ignore the inline "python command" form >> here. */ >> *command = build_command_line (python_control, ""); >> } >> - else if (command_name_equals (cmd, "compile")) >> + else if (command_name_equals_not_inline (p_start, p_end, >> "compile")) >> { >> /* Note that we ignore the inline "compile command" form >> here. */ >> *command = build_command_line (compile_control, ""); >> (*command)->control_u.compile.scope = COMPILE_I_INVALID_SCOPE; >> } >> - >> - else if (command_name_equals (cmd, "guile")) >> + else if (command_name_equals_not_inline (p_start, p_end, >> "guile")) >> { >> /* Note that we ignore the inline "guile command" form here. */ >> *command = build_command_line (guile_control, ""); >> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog >> index b7462a5..ef46a5d 100644 >> --- a/gdb/testsuite/ChangeLog >> +++ b/gdb/testsuite/ChangeLog >> @@ -1,3 +1,8 @@ >> +2017-06-29 Sergio Durigan Junior >> + >> + PR cli/21688 >> + * gdb.python/py-cmd.exp (test_pr21688): New procedure. Call it. >> + >> 2017-06-28 Doug Gilmore >> >> PR gdb/21337 >> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp >> b/gdb/testsuite/gdb.python/py-cmd.exp >> index 2dbf23ce..052afa4 100644 >> --- a/gdb/testsuite/gdb.python/py-cmd.exp >> +++ b/gdb/testsuite/gdb.python/py-cmd.exp >> @@ -181,6 +181,36 @@ gdb_test "complete expr_test bar\." \ >> "expr_test bar\.bc.*expr_test bar\.ij.*" \ >> "test completion through complete command" >> >> +# Testing PR cli/21688. This is not language-specific, but the >> +# easiest way is just to test with Python. >> +proc test_pr21688 { } { > > I am not a fan of naming procs and documenting things solely based on > PR numbers, it's cryptic and requires to go check the web page to know > what this is for. I'd prefer a short description (e.g. Test that the > "python" command is correctly recognized as an inline or multi-line > command when entering a sequence of commands, something like that) and > an appropriate name. Mentioning the PR in the comment is still good > though, if the reader wants to know the context in which this was > added. Sure thing, I'll change the proc name and make sure to mention the PR in the comments. > >> + set define_cmd_not_inline { >> + { "if 1" " >$" } >> + { "python" " >$" } >> + { "print ('hello')" " >$" } >> + { "end" " >$" } >> + { "end" "hello\r\n" } } >> + >> + set define_cmd_inline { >> + { "if 1" " >$" } >> + { "python print ('hello')" " >$" } >> + { "end" "hello\r\n" } } >> + >> + foreach t [list $define_cmd_not_inline $define_cmd_inline] { >> + foreach l $t { >> + foreach { command regex } $l { > > I didn't understand this last foreach at first, but IIUC it's for > unpacking $l? An alternative that might be clearer is lassign: > > lassign $l command regex Yeah, it is for unpacking $l. Indeed, lassign makes it clearer, I'll use that. > >> + gdb_test_multiple "$command" "$command" { > > Watch out, this will make some test names that end with parenthesis, > and a few of them will be non-unique. Hm, good point. I'll review the test messages. I'll send a v2 soon. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/