This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] i386/amd64 h/w watchpoints in gdbserver


On Mon, Jun 29, 2009 at 1:46 PM, Pedro Alves<pedro@codesourcery.com> wrote:
>> >> +/* Support for h/w breakpoints.
>> >> + ? This support is not currently used, kept for reference. ?*/
>> >
>> > Any reason for not using this currently? ?If there's a good reason,
>> > than let's drop it. ?But I'd prefer to have it working. ?:-)
>>
>> deleted.
>
> Now you left me curious as to what was missing.

For reference sake, it was deleted because using it would mean adding
new features to the patch.  I wanted to keep the focus on the task at
hand.   The additions are for a different feature, and it can come in
a later patch.

>> >> Index: utils.c
>> >> ===================================================================
>> >
>> > +char *
>> > +paddr (CORE_ADDR addr)
>> >
>> > This isn't documented in neither server.h or here?
>>
>> Just "going with the flow".
>
> The flow says: "look closer and you'll see that all functions
> in utils.c have description comments."

It wasn't utils.c that I was referring to. Comments added.

> I don't think a Win64 port of gdbserver will take too long
> to appear, so I'm sure this will an issue (albeit small, as this is
> debug output). ?Note that casting a pointer to long as never been
> garanteed to be portable. ?With the other point, all I meant
> was to look here:
>
> ?http://sourceware.org/ml/gdb-patches/2009-06/msg00223.html
>
> and notice that paddr is gone, in favor of paddress. ?So, it seemed
> to be that if copying an interface from GDB, might as well copy
> the one that is going to stay.

Righto.
The new paddress in gdb has a gdbarch argument which gdbserver
currently doesn't have.
I elided it.  I hope that's ok.

>> I went with something
>> simple that works for now.
>> IWBN if this kind of thing were in, say, libiberty and tools could just use it.
>
> Fine, but this is not really an argument. ?You'd already brought a bit of
> code over from GDB, it's just a matter of bringing in more bits. ? OTOH, for
> gdbserver, it wouldn't probably be a problem to just use %p instead. ?But
> I'm fine with going with cast to long for now. ?It was a "Note:"
> afterall. ?Someone else will have to worry about it.

The nice thing about wrapping a particular portability issue in one
place is that, well, it's in one place.  :-)  There are several places
in gdbserver that currently use (long) for printing CORE_ADDRs.

As far as being a "Note",  I wonder if we need a convention for review
comments that are mandatory versus review comments that are just
notes, lest future reviews and responses interpret them wrongly (in
either direction).

>> >> + ?/* Target-specific additions. ?*/
>> >
>> > Warning: "Target" overload. ?We need to get into the habit
>> > of not doing this --- it makes refering to these things quite
>> > ambiguous. ?Call it "arch" or something else. ?There are other
>> > similar cases.
>>
>> I dunno. ? there's "the_low_target" in linux-low.h
>> Perhaps we can migrate away but I don't see the above "infraction" as
>> being critical.
>
> Please! ?Could spare us these extra iterations and go with
> "Low-target specific additions" then, or some other 4 or 5
> letters adjustment, right? ?;-) ?I wasn't asking for a rewrite.

'k.  I went with arch-specific.

>> >> + ?/* ??? Will need tweaking for multi-process. ?*/
>> >
>> > Indeed. ?Why not just set the debug_registers_changed in lwps
>> > of the current process?
>>
>> Are there any existing examples of this?
>> I would have done that had process_info contained the list of its
>> threads (it would have been trivially straightforward).
>
> Just match the pid of the current process with the pid of
> each thread? ?linux-low.c does that in several places.

'k.

>> + ?return 0; /* ??? fatal? ?*/
>
> This just means not-stopped-by-watchpoint? ?Certainly not fatal.

The context here is that the function isn't called unless
stopped_by_watchpoint has returned true.  Thus if stopped_data_address
then can't find the address something is wrong.
I've been thinking that stopped_by_watchpoint and stopped_data_address
should be combined, but it's not clear one would always want that.

> Right. ?An extra point:
>
> On Tuesday 23 June 2009 08:37:14, Doug Evans wrote:
>> + ? ?default:
>> + ? ? ?error ("Z_packet_to_hw_type: bad watchpoint type %c", type);
>
> This should not call error, but return unsupported.

This is in a subroutine of functions defined to only handle
watchpoints.  "Not supported" is handled at a higher level.  I changed
"error" to "fatal" to make this more clear.

>> how about this?
>
> It looks goodish, but I'd really like to see the points I
> raise be addressed, instead of just ignored. ?It just makes
> us waste the (narrow already) review bandwidth...

I disagree with the categorization of having ignored those comments.
[Plus I did apply roughly 15 of 19 comments without question.  1/2 :-)]
But no worries.
Humble apologies, and noted for future reference.

Pierre, nothing much has changed win32-wise except some functions got renamed.
I don't know if you want to first give this a spin again or not.

Attachment: gdb-090630-gdbserver-hw-wp-5.patch.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]