From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22205 invoked by alias); 5 Apr 2013 16:07: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 22195 invoked by uid 89); 5 Apr 2013 16:07:09 -0000 X-Spam-SWARE-Status: No, score=-8.9 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 05 Apr 2013 16:07:06 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r35G72UF008911 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 5 Apr 2013 12:07:02 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r35G70aQ014445; Fri, 5 Apr 2013 12:07:00 -0400 Message-ID: <515EF6A3.2080704@redhat.com> Date: Fri, 05 Apr 2013 18:30:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Keith Seitz CC: Hui Zhu , Hui Zhu , gdb-patches ml Subject: Re: [PATCH] Fix dprintf work not right if it is pending References: <514BF736.3070706@mentor.com> <514C3C85.4000704@codesourcery.com> <514EEBFF.8090705@redhat.com> <5154378D.60302@redhat.com> <515B1DF7.3090705@redhat.com> In-Reply-To: <515B1DF7.3090705@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00131.txt.bz2 Hello, Thanks for the help Keith. Much appreciated. On 04/02/2013 07:05 PM, Keith Seitz wrote: > On 03/29/2013 12:42 AM, Hui Zhu wrote: > >>>> + breakpoint_re_set_default (b); >>>> + >>>> + if (b->extra_string != NULL) >>>> + update_dprintf_command_list (b); You shouldn't be able to create a dprintf without an extra string, right? But, we can't get to the extra string until the breakpoint's location is pending, so we couldn't check when the breakpoint was created. $ ./gdb -q -nx (gdb) dprintf pendfunc No symbol table is loaded. Use the "file" command. Make dprintf pending on future shared library load? (y or [n]) y Dprintf 1 (pendfunc) pending. (gdb) info breakpoints Num Type Disp Enb Address What 1 dprintf keep y pendfunc (gdb) Ok, now let's load symbols. (gdb) file ./testsuite/gdb.base/dprintf-pending Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/dprintf-pending...done. and: (gdb) info breakpoints Num Type Disp Enb Address What 1 dprintf keep y 0x0000000000400560 the location resolved. But, notice no commands attached... (gdb) start Temporary breakpoint 2 at 0x400690: file ../../../src/gdb/testsuite/gdb.base/dprintf-pending.c, line 26. Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/dprintf-pending Temporary breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.base/dprintf-pending.c:26 26 pendfunc (3); /* break main here */ (gdb) n (gdb) info breakpoints Num Type Disp Enb Address What 1 dprintf keep y 0x00007ffff7dfc69d in pendfunc at ../../../src/gdb/testsuite/gdb.base/dprintf-pendshr.c:27 breakpoint already hit 1 time (gdb) I think we want this: static void dprintf_re_set (struct breakpoint *b) { breakpoint_re_set_default (b); /* This breakpoint could have been pending, and be resolved now, and if so, we should now have the extra string. If we don't, the dprintf was malformed when created, but we couldn't tell because we can't extract the extra string until the location is resolved. */ if (b->loc != NULL && b->extra_string == NULL) error (_("Format string required")); if (b->extra_string != NULL) update_dprintf_command_list (b); } Please add a test for this. >>>> +} >>>> + >>> >>> >>> This will update the command list every time breakpoints are reset and could >>> be limited to only those needing updating. Is there perhaps a reason to >>> always do this? You mean, only update the command list if there isn't one before (because the breakpoint was pending before) ? >> >> I think it need, because it need to generate different commands with >> different status for example: >> if (target_can_run_breakpoint_commands ()) >> printf_line = xstrprintf ("agent-printf %s", dprintf_args); >> > > I'm not understanding this example. How is this likely to change whenever breakpoints are reset? Is there perhaps a way to add a test to demonstrate this requirement? I think what he's saying is, even independently of issues with pending dprintf breakpoints, if you, in the same gdb run: 1 - connect to target 1, that can run breakpoint commands. 2 - create a dprintf, which resolves fine. 3 - disconnect from target 2 4 - connect to target 2, that can NOT run breakpoint commands. After steps #3/#4, you'll want the dprintf command list to be updated, because target 1 and 2 may well return different answers for target_can_run_breakpoint_commands(). Given absence of finer grained resetting, we get to do it all the time. On 04/04/2013 02:29 PM, Hui Zhu wrote: > +# Restart with a fresh gdb. > + > +gdb_exit > +gdb_start > +gdb_reinitialize_dir $srcdir/$subdir > + > +gdb_load ${binfile} Use clean_restart here. > +gdb_test_multiple "dprintf pendfunc1, \"x=%d\\n\", x" "set pending dprintf" { > + -re ".*Make dprintf pending.*y or \\\[n\\\]. $" { > + gdb_test "y" "Dprintf.*pendfunc1.*pending." "set pending dprintf" > + } > +} > + gdb_test has built-in support for questions. Write these sorts of things as: gdb_test \ "dprintf pendfunc1, \"x=%d\\n\", x" \ "Dprintf.*pendfunc1.*pending." \ "set pending dprintf (without symbols)" \ ".*Make dprintf pending.*y or \\\[n\\\]. $" \ "y" There's at least one more instance. > +if { [skip_gdbserver_tests] } { > + return 0 > +} > + > +# Get warning or no output is OK. > +gdb_test "set dprintf-style agent" ".*" "Set dprintf style to agent" > + > +gdbserver_run "" I'd much prefer remove this skip_gdbserver_tests check, and this gdbserver_run. IOW, keep running the test against the target the current board is set up with. There are remote servers other than GDBserver out there. > +# Get warning or no output is OK. > +gdb_test "set dprintf-style agent" ".*" "Set dprintf style to agent" What warning would that be? This here?: else if (strcmp (dprintf_style, dprintf_style_agent) == 0) { if (target_can_run_breakpoint_commands ()) printf_line = xstrprintf ("agent-printf %s", dprintf_args); else { warning (_("Target cannot run dprintf commands, falling back to GDB printf")); printf_line = xstrprintf ("printf %s", dprintf_args); } } > + > +gdbserver_run "" > + > +gdb_test "info break" ".*agent-printf \"x=%d\\\\n\", x" \ If that warning triggers, then this will fail... In fact, you should see that when you remove the gdbserver bits. Please make the "set" test check explicitly either no output, or the warning, and then the "info break" test check the corresponding expected output. Then please make sure the test passes with native, and also the native-gdbserver and native-extended-gdbserver boards. It fails with the native-gdbserver board, because the program exists and gdbserver exits before the "set dprintf-style agent". You'll need to add something to prevent that. -- Pedro Alves