Re: [COMMIT] Hardware watchpoints for new inf-ttrace.c module

[Mark Kettenis <kettenis at gnu dot org>]

   Date: Sat, 04 Dec 2004 14:37:40 +0200
   From: "Eli Zaretskii" <eliz@gnu.org>

   > Date: Fri, 3 Dec 2004 23:24:52 +0100 (CET)
   > From: Mark Kettenis <kettenis@gnu.org>
   > 
   > Virtually all of the HP-UX-specific handling is moved within this
   > module, so I expect that whenever we switch, quite a few ugly hacks
   > can go.

   It would be good to have a short description of this machinery in
   gdbint.texinfo.  Right now, it describes only the x86 way of
   implementing hardware-assisted watchpoints.

I can give that a try.  In principle, the HP-UX way of implementing
watchpoints is pretty generic, and could work on any system that
allows GDB to fiddle with memory page protetections.  As such, I think
this would indeed be good information to have in the internals manual.

(Note that this code is just a clean re-implementation of code that's
already present in infttrace.c.)

   > +/* Add the page at address ADDR for process PID to the dictionary.  */
   > [...]
   > +/* Insert the page at address ADDR of process PID to the dictionary.  */
   > +
   > +static void
   > +inf_ttrace_insert_page (pid_t pid, CORE_ADDR addr)
   > +{
   > +  struct inf_ttrace_page *page;
   > +
   > +  page = inf_ttrace_get_page (pid, addr);
   > +  if (!page)
   > +    page = inf_ttrace_add_page (pid, addr);
   > +
   > +  page->refcount++;
   > +}

   Hmmm... wouldn't it be better to have just one function that adds an
   address's page to the dictionary?  Or do you see a situation where a
   call to inf_ttrace_add_page will not be immediately followed by
   incrementing page->refcount?  I generally find it undesirable to have
   two or more functions whose names and purpose comments are synonyms
   ("add page" and "insert page").  It is confusing for a programmer who
   needs to use the functionality, and usually forces to read the code to
   understand how to DTRT.

Yes, it is somewhat confusing, although I don't really see how I can
avoid having two functions without duplicating code.  Perhaps I can
rename the functions or tweak the comments a bit to make this clearer.
Thanks for the heads-up.

   > +static int
   > +inf_ttrace_can_use_hw_breakpoint (int type, int len, int ot)
   > +{
   > +  return (type == bp_hardware_watchpoint);
   > +}

   I was going to ask why not try to support rwatch and awatch, but then
   I realized that you cannot implement target_stopped_data_address, and
   that in turn made it clear that gdbint.texinfo is inaccurate when it
   describes the watchpoint-related primitives.  I will fix the manual
   shortly.

Didn't realize that.  I might be able to implement
target_stopped_data_address though, although there are some 32x64-bit
cross-debugging issues here.  I haven't really looked into it yet
though, since my primary goal is to implement everything that the
current HP-UX native GDB supports.

   > +static int
   > +inf_ttrace_region_size_ok_for_hw_watchpoint (int len)
   > +{
   > +  return 1;
   > +}

   Hmmm... I have a minor issue with this.  Inserting a watchpoint might
   mean that you need to add a page to the dictionary, which in turn
   could fail because there's not enough memory available to GDB.  You
   add a page by using xmalloc, so if there isn't enough memory, GDB will
   exit.  Isn't it better to fail the watchpoint insertion in that case
   rather than abort the entire session?  I realize that
   inf_ttrace_region_size_ok_for_hw_watchpoint is probably not the place
   where such a situation should be handled, but perhaps
   inf_ttrace_add_page or inf_ttrace_insert_watchpoint should be modified
   to do that?

Don't think this is worth it.  The generic breakpoint machinery needs
bits of memory too.  It's probably just as likely that it will fail as
well.

Anyway, thanks for the comments.

Mark