This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC stub-side break conditions 3/5] GDB-side changes
- From: Luis Machado <luis_gustavo at mentor dot com>
- To: Tom Tromey <tromey at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 10 Jan 2012 09:12:57 -0200
- Subject: Re: [RFC stub-side break conditions 3/5] GDB-side changes
- References: <4F05BA10.3090107@mentor.com> <m3aa601p6q.fsf@fleche.redhat.com>
- Reply-to: "Gustavo, Luis" <luis_gustavo at mentor dot com>
Hi Tom,
On 01/06/2012 07:44 PM, Tom Tromey wrote:
Luis> I ran the testsuite and had no additional failures.
I think the patches should include some tests for the new feature.
Yes. I still need to think about those.
I'll probably add a few GDB-side tests for safety, but mostly
target-side ones like the tracepoint tests.
Luis> I'd like to hear about the change to "info break" and also the way we
Luis> should pass conditions down to the stub.
I looked at this code and, while I think the output bits are fine, there
is something I don't understand:
+ /* Print whether the remote stub is doing the breakpoint's condition
+ evaluation. If GDB is doing the evaluation, don't print anything. */
+ if (loc&& is_breakpoint (b)&& loc->cond_bytecode
+ && breakpoint_condition_evaluation_mode ()
+ != condition_evaluation_gdb)
(First, there should be parens around that last subexpression.)
Fixed.
If the user changes 'condition-evaluation', then the new setting might
affect what "info break" prints. But this seems weird, since what is
relevant is what is actually going on under the hood.
Or, on the other hand, changing 'condition-evaluation' should change the
state of all breakpoints; but this requires additional code AFAICT.
This has been addressed according to Eli's and Pedro's comments.
If the user switches to a different mode, we will take appropriate
action. Either we remove conditions or we add them to the target.
Luis> +int remote_supports_cond_breakpoints (void);
Is it much uglier to rearrange things so no forward declaration is
needed?
In this case, unfortunately, i think so. I'd have to move the code that
adds the conditions to the packet buffer to the far bottom of the file.
If keeping similar functions apart is no issue, i can do this change.
Luis> + /* List of conditions the target should evaluate if it supports target-side
Luis> + breakpoint conditions. */
Luis> + struct condition_list *cond_list;
It seems that the callee must free these. I think that should be
documented here.
This should be addressed now with the use of VEC's. We have a vector of
agent expressions.
Luis> + /* Conditional expression in agent expression
Luis> + bytecode form. This is used for stub-side breakpoint
Luis> + condition evaluation. */
Luis> + struct agent_expr *cond_bytecode;
Luis> +
Luis> + /* Signals that the condition has changed since the last time
Luis> + we updated the global location list. This means the condition
Luis> + needs to be sent to the target again. This is used together
Luis> + with target-side breakpoint conditions. */
Luis> + int condition_changed;
Luis> +
Luis> + /* Signals that breakpoint conditions need to be re-synched with the
Luis> + target. This has no use other than target-side breakpoints. */
Luis> + int needs_update;
I still wish we had 'bool'. Maybe this year.
pahole says there are holes in bp_location on my x86-64 box. I think it
is somewhat preferable to fill the holes and to follow existing
practice, that is, use 'char' for boolean fields here.
My preference would be to use 'unsigned int : 1' for boolean fields, but
I don't know how others feel about this.
I'm fine with either of those. I'll switch to char meanwhile, but will
use bitfields if OK.
Thanks!
I'll send an updated patch soon, cc-ing everyone.
Luis
lgustavo@codesourcery.com