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

["Eli Zaretskii" <eliz at gnu dot 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.

> +/* 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.

> +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.

> +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?