From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id B647C3858D37 for ; Mon, 10 Oct 2022 14:04:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B647C3858D37 Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-584-MbA1KndTOsq638Xntdn8Og-1; Mon, 10 Oct 2022 10:04:45 -0400 X-MC-Unique: MbA1KndTOsq638Xntdn8Og-1 Received: by mail-wr1-f69.google.com with SMTP id e11-20020adfa74b000000b0022e39e5c151so2765441wrd.3 for ; Mon, 10 Oct 2022 07:04:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=jaOha0+vW6Sw1aiCSjQYoeLEeJkUx7kiynXlQ533Vy4=; b=qu7LJYldTeOhdketejwJnTaCNLsfP6thRY1n0uf2rYhPCVaBzYc+Gk5JG1bbKwZPkf EAIVXHlCWtM/WGzPduSU1L9+wTeVBdzdVeStMLHO2+GqlJpNGG9ggl3A/+ycNMmNYKMv GGwIcQ35S+AFImvk+EFusruz7zmPeqx3CoMTtq3AxZdzCRfiPaUqjT3LukyE5ldQ+gSe 6UMo15dMHylpTZipTio/rW1aHZDVT9h2s69khvYkuWFLwxqarbQRlvLYIMsBdJxINl9U 0g/NS20/oULrWUxyskthyYYuz2WbsvUl9QR4WF/sRw0O27+Pg9HQU0rGQOlqZK8rk7Mh bUPQ== X-Gm-Message-State: ACrzQf1+1+zObPC2MdeIFW8oy9RbCCkTsZstThS/7yub9JFxvYSfEfIR uVbgiOxCgPNhj41xcFoRsmB3cj+eBQ0dhaQuhhG1DqRyWroPl4El9iXp3RZ65HvNnb3OIiXbSUL YZkcK11zCBDt6Aaa/L9F+8Q== X-Received: by 2002:a05:600c:3582:b0:3c2:7002:2cb0 with SMTP id p2-20020a05600c358200b003c270022cb0mr12784759wmq.170.1665410683693; Mon, 10 Oct 2022 07:04:43 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5+74xAbhorAxX6u1cemre/3z2shnnIA/k1D4/FKnVtCNX+L6VZFBhSX4RB4vJKAr2ni+7u9Q== X-Received: by 2002:a05:600c:3582:b0:3c2:7002:2cb0 with SMTP id p2-20020a05600c358200b003c270022cb0mr12784736wmq.170.1665410683430; Mon, 10 Oct 2022 07:04:43 -0700 (PDT) Received: from localhost (52.72.115.87.dyn.plus.net. [87.115.72.52]) by smtp.gmail.com with ESMTPSA id l12-20020a05600c2ccc00b003b47e8a5d22sm13120862wmc.23.2022.10.10.07.04.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Oct 2022 07:04:43 -0700 (PDT) From: Andrew Burgess To: Tom de Vries , Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception In-Reply-To: References: <15cf0c9f-81ae-9bc8-79ba-e5b4eb1f0412@simark.ca> <87wn9ae25u.fsf@redhat.com> <9ea099ee-e984-63ad-0400-dd9a6f9fa6ef@simark.ca> <87tu4cdnws.fsf@redhat.com> <32ac73a1-c0b8-75cf-72d3-bd11471d0fe6@suse.de> <87o7ukdj47.fsf@redhat.com> Date: Mon, 10 Oct 2022 15:04:42 +0100 Message-ID: <87fsfvepvp.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Mon, 10 Oct 2022 14:04:48 -0000 Tom de Vries writes: > On 10/10/22 13:16, Andrew Burgess wrote: >> Tom de Vries writes: >> >>> On 10/10/22 11:32, Andrew Burgess via Gdb-patches wrote: >>>> Simon Marchi writes: >>>> >>>>> On 2022-10-08 12:00, Andrew Burgess wrote: >>>>>> Simon Marchi writes: >>>>>> >>>>>>>> +# Check that, if the user is using Python Pygments for disassembler >>>>>>>> +# styling, then the styling correctly switches off when an error is >>>>>>>> +# detected in the Python code. >>>>>>>> +proc test_disassembler_error_handling { } { >>>>>>>> + >>>>>>>> + # This test requires the Python Pygments module to be installed >>>>>>>> + # and used by GDB. >>>>>>>> + if { !$::python_disassembly_styling } { >>>>>>>> + return >>>>>>>> + } >>>>>>>> + >>>>>>>> + save_vars { env(TERM) } { >>>>>>>> + # We need an ANSI-capable terminal to get the output. >>>>>>>> + setenv TERM ansi >>>>>>>> + >>>>>>>> + # Restart GDB with the correct TERM variable setting, this >>>>>>>> + # means that GDB will enable styling. >>>>>>>> + clean_restart_and_disable "restart 4" $::binfile >>>>>>>> + >>>>>>>> + # Disable use of libopcodes for styling. As this function is >>>>>>>> + # only called when Python Pygments module is available, we >>>>>>>> + # should now be using that module to style the disassembler >>>>>>>> + # output. >>>>>>>> + gdb_test_no_output "maint set libopcodes-styling enabled off" >>>>>>>> + >>>>>>>> + # Disassemble a single instruction and ensure that the output >>>>>>>> + # has styling markers in it. >>>>>>>> + set insn_before [get_single_disassembled_insn] >>>>>>>> + gdb_assert { [regexp "\033" $insn_before] } \ >>>>>>>> + "have style markers when Pygments is working fine" >>>>>>>> + >>>>>>>> + # Now replace the standard function that colorizes the >>>>>>>> + # disassembler output, with a new function that always returns >>>>>>>> + # None, this should cause GDB to stop using the Pygments >>>>>>>> + # module for disassembler styling. >>>>>>>> + gdb_py_test_silent_cmd \ >>>>>>>> + [multi_line_input \ >>>>>>>> + "python" \ >>>>>>>> + "def replacement_colorize_disasm(content,gdbarch):" \ >>>>>>>> + " return None" \ >>>>>>>> + "gdb.styling.colorize_disasm = replacement_colorize_disasm" \ >>>>>>>> + "\004"] \ >>>>>>> >>>>>>> Any reason you are using \004 here, instead of end? I don't quite >>>>>>> understand why, but it seems to cause some random failures. Running the >>>>>>> test under `taskset -c 2` makes it fail most of the time. Running it >>>>>>> with check-read1 makes it fail consistently: >>>>>>> >>>>>>> FAIL: gdb.base/style.exp: capture_command_output for x/1i *main >>>>>>> >>>>>>> When changing \004 for end, it passes. I don't have an explanation why >>>>>>> though. >>>>>> >>>>>> It'll be a copy&paste. There's a couple of other places in the >>>>>> testsuite where this pattern is used. >>>>>> >>>>>> The patch below changes all three to use 'end'. How's this? >>>>>> >>>>>> Thanks, >>>>>> Andrew >>>>> >>>>> This is fine with me, but again, I didn't dig into it so I don't really >>>>> know what sending a \004 was causing, >>>> >>>> Hi, >>>> >>>> Given you were seeing problems with the use of \004, and the tests >>>> definitely were not about whether \004 could be used or not, I've pushed >>>> the patch I proposed (exact version is below). >>>> >>> >>> I found this independently and filed a PR ( >>> https://sourceware.org/bugzilla/show_bug.cgi?id=29664 ) and wrote the >>> same fix (in the same three test-cases), and was about to commit ... >>> >>> The FAIL is caused by using gdb_py_test_silent_cmd, which adds a "\n" at >>> the end of the command in combination with "\004", causing a stray "\n" >>> that generates an extra prompt, which makes the prompt matching >>> unpredictable. >> >> Apologies for the duplicated effort. Also, thanks for the explanation, >> which, if I'd spotted sooner would have saved me investigating the >> problem. >> >> Again, thanks for picking up after me, and apologies for wasting your >> time. >> > > Hi Andrew, > > thanks for following up. > > Luckily most of my time went to investigating the root cause (which had > me puzzled for quite a bit), so that's not effort wasted. > > Also, I forgot to check the mailing list to see if the FAIL was already > noted there, so that's on me. > > Anyway, I propose to error out in gdb_test_multiple on this specific > pattern. Dealing with ^C / ^D for now. > > Not tested yet (I'll start testing in now in combination with the > proposed patch to fix the build). > > WDYT? This seems like a sensible precaution, I like it. Patch looks good. Thanks, Andrew > > Thanks, > - Tom > [gdb/testsuite] Detect trailing ^C/^D in command > > Detect a trailing ^C/^D in the command argument of gdb_test, and error out. > > Tested on x86_64-linux. > > --- > gdb/testsuite/gdb.testsuite/gdb-test.exp | 34 ++++++++++++++++++++++++++------ > gdb/testsuite/lib/gdb.exp | 4 ++++ > 2 files changed, 32 insertions(+), 6 deletions(-) > > diff --git a/gdb/testsuite/gdb.testsuite/gdb-test.exp b/gdb/testsuite/gdb.testsuite/gdb-test.exp > index 2ce8eb31d73..e891f81e7b0 100644 > --- a/gdb/testsuite/gdb.testsuite/gdb-test.exp > +++ b/gdb/testsuite/gdb.testsuite/gdb-test.exp > @@ -19,10 +19,32 @@ clean_restart > > # Check that a command with trailing newline triggers an error. > > -set results [catch { > - gdb_test "pwd\n" ".*" "cmd with trailing newline" > -} output] > +with_test_prefix "cmd with trailing newline" { > + set results [catch { > + gdb_test "pwd\n" ".*" "pwd" > + } output] > > -gdb_assert { $results == 1 } > -set expected_error_msg "Invalid trailing newline in \"pwd\n\" command" > -gdb_assert { [string equal $output $expected_error_msg] } > + gdb_assert { $results == 1 } > + set expected_error_msg "Invalid trailing newline in \"pwd\n\" command" > + gdb_assert { [string equal $output $expected_error_msg] } > +} > + > +with_test_prefix "cmd with trailing control code" { > + foreach_with_prefix control_code {^C ^D} { > + switch $control_code { > + ^C { > + set cmd "\003" > + } > + ^D { > + set cmd "\004" > + } > + } > + set results [catch { > + gdb_test $cmd ".*" "control code" > + } output] > + > + gdb_assert { $results == 1 } > + set expected_error_msg "Invalid trailing control code in \"$cmd\" command" > + gdb_assert { [string equal $output $expected_error_msg] } > + } > +} > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp > index 5f0acfaa530..ac28ede1b08 100644 > --- a/gdb/testsuite/lib/gdb.exp > +++ b/gdb/testsuite/lib/gdb.exp > @@ -1003,6 +1003,10 @@ proc gdb_test_multiple { command message args } { > error "Invalid trailing newline in \"$command\" command" > } > > + if [string match "*\[\003\004\]" $command] { > + error "Invalid trailing control code in \"$command\" command" > + } > + > if [string match "*\[\r\n\]*" $message] { > error "Invalid newline in \"$message\" test" > }