Re: [PATCH 1/4] Support the new BookE ptrace interface

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

On Tue 29 Dec 2009 04:11:41 Joel Brobecker wrote:
> Overall, this looks OK to me.  A few general, non-blocking comments:
>   (2) Again, non-blocking, but I'm left wondering if the "old" case
>       (when we don't have the new interface available), could be made
>       a special case of the new one.  For instance, you maintain two
>       sets of globals, one used for booke, and one used when non-booke.
>       Would it be possible to maintain just one set that works for booke,
>       and then infer the non-booke ones if necessary? Or said another
>       way, in the non-booke case, we can still maintain the same data
>       structures as in the booke case, and we can then convert that
>       data to whatever we need in the non-booke case.  The only time
>       I think this would be needed is during insertion/deletion.
>       I think that this might simplify a bit some of the code, avoid
>       maintaining two exclusive sets of globals, and this might also
>       help with the transition away from the "old" interface when we
>       decide not to support it anymore.

Well, the only global that the old interface uses is saved_dabr_value. :-)
But I see your point. I'll play with it and see what I get.

> Has Ulrich had a chance to look at these patches? As I said, they look
> OK to me, and are pre-approved pending the little nits I mentioned below.
> But this is a rather large change, and it would be nice if someone more
> knowledgeable with the ppc than me also had a look too.

Unfortunately Ulrich didn't have a chance to look at them yet, and I know he's 
on vacation so it might take a while... :-/

> The comments above can be discussed as followup patches if you like.

Since you mention it, I'd like to address the old-interface-as-special-case-
of-new-one suggestion in a followup patch.

> > +/*
> > + * features will have bits indication whether there is support for:
> > + */
> 
> Small nit: We avoid the star ('*') at the beginning of the line (and
> the sentence should start with a capital letter):

Right. Fixed.

> /* Features will have bits indication whether there is support for:  */
> 
> The sentence itself does not sound too English to me, but I'm OK with it
> anyways. I think we get the idea :).

I corrected from "indication" to "indicating".

> (there are a few more instances of the extra stars in that same section).

Fixed those too.

> > +struct hw_break_tuple {
> > +  long slot;
> > +  struct ppc_hw_breakpoint *hw_break;
> > +};
> 
> Another small formatting nit:
> 
> struct hw_break_tuple
> {
>   long slot;
>   struct ppc_hw_breakpoint *hw_break;
> };

Oops. Fixed.

> > +  if (!have_ptrace_new_debug_booke)
> > +    {
> > +      ptid_t ptid;
> > +      int tid;
> > +
> > +      /* We need to know whether ptrace supports PTRACE_SET_DEBUGREG and
> > whether +	 the target has DABR.  If either answer is no, the ptrace call
> > will +	 return -1.  Fail in that case.  */
> > +      tid = TIDGET (ptid);
> > +      if (tid == 0)
> > +	tid = PIDGET (ptid);
> > +
> > +      if (ptrace (PTRACE_SET_DEBUGREG, tid, 0, 0) == -1)
> > +	return 0;
> 
> I'm ok with the implementation as is, but I'm wondering if this is
> the type of data that we'd want to cache. Maybe this makes things
> more complicated than it is worth?

That is just how GDB does things now. I'd like to change the current old 
interface code as little as possible for now, since we are making a big change 
overall.

> > +/* This function can be used to retrieve a thread_points by
> > +   the TID of the related process/thread.  If nothing has been
> > +   found, it returns NULL.  */
> 
> Can you document the behavior when ALLOC_NEW is non-zero if nothing
> has been found?

Done:

/* This function can be used to retrieve a thread_points by the TID of the
   related process/thread.  If nothing has been found, and ALLOC_NEW is 0,
   it returns NULL.  If ALLOC_NEW is non-zero, a new thread_points for the
   provided TID will be created and returned.  */

> > +  if ((slot = ptrace (PPC_PTRACE_SETHWDEBUG, tid, 0, p)) < 0)
> 
> Hmmm, Let's avoid assignments inside conditions. It's harder to read, IMO.
> 
>     slot = ptrace (PPC_PTRACE_SETHWDEBUG, tid, 0, p);
>     if (slot < 0)

Right. Fixed.

> > +    perror_with_name (_("\n\
> > +Please review your watchpoints/breakpoints and make sure the mask,
> > ranges \n\ +and addresses are within acceptable limits."));
> 
> Is this something that we could catch earlier, when the *point is
> created by the user?

That message is really just in case the ptrace call fails. The breakpoints and 
watchpoints should be correct by now. I changed the message to:

  perror_with_name (_("Unexpected error setting breakpoint or watchpoint"));

> > +    perror_with_name (_("\n\
> > +Could not delete the breakpoint/watchpoint."));
> 
> All the other calls to perror_with_name do not insert a newline
> at the beginning of the error string.  The previous one was rather long,
> so I thought it made sense. But this one, it seems more consistent to
> not have it. WDYT?

Agreed. I changed it to a similar message as the previous one:

  perror_with_name (_("Unexpected error deleting breakpoint or watchpoint"));

> > +      struct ppc_hw_breakpoint p;
> > +
> > +      p.version         = PPC_DEBUG_CURRENT_VERSION;
> > +      p.trigger_type    = PPC_BREAKPOINT_TRIGGER_EXECUTE;
> > +      p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
> > +      p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
> > +      p.addr            = (uint64_t) bp_tgt->placed_address;
> > +      p.addr2           = 0;
> > +      p.condition_value = 0;
> > +
> > +      booke_remove_point (&p, TIDGET (ptid));
> 
> It seemed a bit strange that you're using the entire ppc_hw_breakpoint
> as a key to your hw_breaks_structure. But thinking more about it, I
> don't know if you have many options or not. At first, I thought it would
> be a simple matter of remembering the slot where the point was inserted,
> but then you'd need a map between the breakpoint and the slot.  The
> current interface is too awkward for that (we only get passed the
> bp_target_info). I have the same problem on VxWorks, where created
> a map to be able to store some target-specific private date alongside
> each breakpoint.
> 
> Oh well...

I agree that the data structures are a bit convoluted, but I can't come up 
with anything better for now. If someone has suggestions, I'l be glad to 
consider them. :-)

> > +#define HW_WATCH_RW_TRIGGER(t, rw) \
> > +  if (rw == hw_read) \
> > +    t = PPC_BREAKPOINT_TRIGGER_READ; \
> > +  else if (rw == hw_write) \
> > +    t = PPC_BREAKPOINT_TRIGGER_WRITE; \
> > +  else \
> > +    t = PPC_BREAKPOINT_TRIGGER_READ | PPC_BREAKPOINT_TRIGGER_WRITE;
> 
> Can we use a function instead. I don't mind macros when it's a litteral
> constant, or a simple arithmetic expression, but this is really complex
> enough that a function would be better (particularly during debugging).

Ok, changed.

> > +  if (have_ptrace_new_debug_booke)
> > +    {
> > +      if (len <= 4)
> > +	ALL_LWPS (lp, ptid)
> 
> What happens if len > 4? Is that even possible? Right now, it looks
> like we're returning zero as if the watchpoint has been inserted,
> even though it hasn't...

Oh, that if should be in the patch for ranged watchpoint support. There, 
ppc_linux_insert_watchpoint assumes that a watchpoint larger than 4 bytes 
should be converted to a ranged watchpoint.

But I don't think that's a good idea anymore, and instead added a 
target_insert_ranged_watchpoint method for GDB to explicitly ask the target to 
create a ranged watchpoint when the user asks for one, or when an array or 
struct is to be watched. The watchpoint length is not used for that anymore.

I'll post a new version of these patches with that change, and the fixes from 
the review comments I got so far. I'll submit them either tomorrow or on 
monday...
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center