On Mon, Jun 29, 2009 at 1:46 PM, Pedro Alves 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.