Re: [RFA] i386/amd64 h/w watchpoints in gdbserver

Pedro Alves
2009-06-29T20:45:45Z

On Tuesday 23 June 2009 08:37:14, Doug Evans wrote:
> On Sat, Jun 20, 2009 at 4:55 PM, Pedro Alves<pedro@codesourcery.com> wrote:
> > On Tuesday 02 June 2009 16:36:05, Doug Evans wrote:
> >
> > I don't think none of these forward declarations is needed?
>
> ok
> [once upon a time, they were ok. the new rules haven't been locked in
> memory yet]

It's easy --- less redundant code to maintain is good. In this case, in addition
to the unnecessary function prototypes, even those large descriptions were
duplicated.

>
> >> +
> >> +int
> >> +i386_low_stopped_by_watchpoint (struct i386_debug_reg_state *state)
> >> +{
> >> +  CORE_ADDR addr = 0;
> >> +  /* NOTE: gdb version passes boolean found/not-found result from
> >> +     i386_stopped_data_address.  */
> >> +  addr = i386_low_stopped_data_address (state);
> >> +  return (addr != 0);
> >> +}
> >
> > Same as above.  You've probably thought about that too...
> >
> >> +
> >> +/* 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.

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

>
> > +{
> > +  char *str = get_cell ();
> > +  xsnprintf (str, CELLSIZE, "%lx", (long) addr);
> >
> >                                     ^^^^
> >
> > Note: this will be wrong on Win64...  BTW, Ulrich
> > was removing several of these functions from GDB
> > in the removing-current_gdbarch series.  Will this one
> > stay?  Might be worth it to use the one that is going
> > to stay in GDB.
>
> I think a higher order bit is that gdb and gdbserver cannot share
> code. Bringing over all the smarts to handle all the different
> portability issues is painful/depressing.

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.

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

>
> >> Index: linux-low.h
> >> ===================================================================
> >> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
> >> retrieving revision 1.30
> >> diff -u -p -r1.30 linux-low.h
> >> --- linux-low.h 12 May 2009 22:25:00 -0000      1.30
> >> +++ linux-low.h 1 Jun 2009 22:02:43 -0000
> >> @@ -56,8 +56,13 @@ struct process_info_private
> >>
> >>    /* Connection to the libthread_db library.  */
> >>    td_thragent_t *thread_agent;
> >> +
> >> +  /* 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.

> >> --- linux-x86-low.c     13 May 2009 19:11:04 -0000      1.2
> >> +++ linux-x86-low.c     1 Jun 2009 22:02:43 -0000
> >
> >> +static unsigned long
> >> +x86_linux_dr_get (ptid_t ptid, int regnum)
> >> +{
> >> +  int tid;
> >> +  unsigned long value;
> >> +
> >> +  tid = TIDGET (ptid);
> >> +  if (tid == 0)
> >> +    tid = PIDGET (ptid);
> >
> > The tid == 0 case is dead code coming from GDB, isn't it?
> > Likewise in other places.
>
> Perhaps. There's similar code in linux-low.c:same_lwp.
> == 0 code deleted.

Yes, but same_lwp really handles cases where
ptid_get_lwp == 0, due to find_lwp_pid. IIUC, these functions
are always called with a full thread ptid.

> >
> >> +/* Update the inferior's debug register REGNUM from STATE.  */
> >> +
> >> +void
> >> +i386_dr_low_set_addr (const struct i386_debug_reg_state *state, int regnum)
> >> +{
> >> +  struct inferior_list_entry *lp;
> >> +  CORE_ADDR addr;
> >> +
> >> +  if (! (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR))
> >> +    fatal ("Invalid debug register %d", regnum);
> >> +
> >> +  addr = state->dr_mirror[regnum];
> >> +
> >> +  /* ??? 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.

> + return 0; /* ??? fatal? */

This just means not-stopped-by-watchpoint? Certainly not fatal.

> Setting aside breakpoints+watchpoints -> "points",
> http://sourceware.org/ml/gdb-patches/2009-06/msg00594.html

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.

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

--
Pedro Alves