[COMMIT] Hardware watchpoints for new inf-ttrace.c module
Sat Dec 4 14:07:00 GMT 2004
Date: Sat, 04 Dec 2004 14:37:40 +0200
From: "Eli Zaretskii" <email@example.com>
> Date: Fri, 3 Dec 2004 23:24:52 +0100 (CET)
> From: Mark Kettenis <firstname.lastname@example.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
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
Anyway, thanks for the comments.
More information about the Gdb-patches