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] |
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] |