From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20484 invoked by alias); 1 Oct 2014 17:52:21 -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 20469 invoked by uid 89); 1 Oct 2014 17:52:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 01 Oct 2014 17:52:19 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s91HqFbJ001333 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 1 Oct 2014 13:52:16 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s91HqEID015223; Wed, 1 Oct 2014 13:52:14 -0400 Message-ID: <542C3F4D.70104@redhat.com> Date: Wed, 01 Oct 2014 17:52:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Nick Bull , gdb-patches@sourceware.org Subject: Re: [PATCH v6] Events when inferior is modified References: <5419C597.4000300@gmail.com> In-Reply-To: <5419C597.4000300@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-10/txt/msg00015.txt.bz2 I see many new functions without intro comments. Please add an intro comment to each new function, describing the contract of the function. If the function is just implementing some hook, then have the function comment say that "Inplements method foo for bar", instead of duplicating comments about parameters and return types. Please add comments to new enums (inferior_call_kind), and their values, etc. too. > @@ -627,6 +628,8 @@ call_function_by_hand (struct value *function, int > nargs, struct value **args) > target_values_type = values_type; > } > > + observer_notify_inferior_call_pre (); > + > /* Determine the location of the breakpoint (and possibly other > stuff) that the called function will return to. The SPARC, for a > function returning a structure or union, needs to make space for > @@ -860,6 +863,8 @@ call_function_by_hand (struct value *function, int > nargs, struct value **args) > e = run_inferior_call (tp, real_pc); > } > > + observer_notify_inferior_call_post (); > + > /* Rethrow an error if we got one trying to run the inferior. */ > > if (e.reason < 0) I see an issue with these events. On the call_pre case, the caller can infer which thread the infcall was being run on from the currently selected thread. But, not so from the call_post. The inferior may have crashed, exited, or stopped for some breakpoint in a different thread at that point. So the user of that event doesn't really know which function call finished. GDB only does one function call at a time currently, but that's subject to change. It'd be best to design with that in mind already. Also, the documentation leaves this issue unspecified: > +@item events.inferior_call_post > +Emits @code{gdb.InferiorCallPostEvent} which indicates that a function in > +the inferior has returned. I, as I reader, immediately wonder what happens if the function doesn't get to return, but inferior exits, etc. E.g., "is the event called in that scenario as well?". > +# Test register changed event > +gdb_test_no_output {set $old_sp = $sp} > +gdb_test {set $sp = 0} ".*event type: register-changed.* > +.*frame: .* > +.*num: .*" > +gdb_test {set $sp = 1} ".*event type: register-changed.* > +.*frame: .* > +.*num: .*" > +gdb_test {set $sp = $old_sp} ".*event type: register-changed.* > +.*frame: .* > +.*num: .*" Please write explicit \r\n instead. (or use e.g., gdb_test_sequence). > +set test "continue to breakpoint 5" > +gdb_test_multiple "continue" $test { > + -re "event type: memory-changed" { > + fail $test > + } > + -re ".*event type: continue.* > +.*event type: stop.* > +.*stop reason: breakpoint.* > +.*all threads stopped.*$gdb_prompt $" { > + pass $test > + } > +} > + > +gdb_test_no_output "delete 5" Please avoid hard coding breakpoint numbers in tests. You can instead extract the number with expect_out when you create the breakpoint, and store it in a tcl variable. Alternatively, use $bpnum. > +gdb_test_multiple {up} $test { > + -re "event type: register-changed" { > + fail $test > + } Please always match $gdb_prompt $, otherwise you're leaving the prompt in the expect buffer, which confuses the next gdb_test_multiple/gdb_test call. While at it, please make sure the test passes with "make check-read1". > + -re "#1.*in first.*\r\n.*do_nothing.*\r\n$gdb_prompt $" { > + pass $test > + } > +} > + ** gdb.events.inferior_call_pre Function call is about to be made. > + ** gdb.events.inferior_call_post Function call has just been made. > + ** gdb.events.memory_changed A memory location has been altered. > + ** gdb.events.register_changed A register has been altered. Maybe it's just me, but I find this hard to read. I'd expect some kind of separator between the event name and its description. Like e.g.,: ** gdb.events.register_changed - A register has been altered. or: ** gdb.events.register_changed: A register has been altered. or: ** gdb.events.register_changed A register has been altered. Thanks, Pedro Alves