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

[Joel Brobecker <brobecker at adacore dot com>]

> 2009-12-23  Sergio Durigan Junior  <sergiodj@linux.vnet.ibm.com>
> 	    Thiago Jung Bauermann  <bauerman@br.ibm.com>
> 
> 	* ppc-linux-nat.c (PTRACE_GET_DEBUGREG): Update comment.
> 	(PPC_PTRACE_GETWDBGINFO, PPC_PTRACE_SETHWDEBUG, PPC_PTRACE_DELHWDEBUG,
> 	ppc_debug_info, PPC_DEBUG_FEATURE_INSN_BP_RANGE,
> 	PPC_DEBUG_FEATURE_INSN_BP_MASK, PPC_DEBUG_FEATURE_DATA_BP_RANGE,
> 	PPC_DEBUG_FEATURE_DATA_BP_MASK, ppc_hw_breakpoint,
> 	PPC_BREAKPOINT_TRIGGER_EXECUTE, PPC_BREAKPOINT_TRIGGER READ,
> 	PPC_BREAKPOINT_TRIGGER_WRITE, PPC_BREAKPOINT_TRIGGER_RW,
> 	PPC_BREAKPOINT_MODE_EXACT PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE,
> 	PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE, PPC_BREAKPOINT_MODE_MASK,
> 	PPC_BREAKPOINT_CONDITION_NONE, PPC_BREAKPOINT_CONDITION_AND,
> 	PPC_BREAKPOINT_CONDITION_EXACT, PPC_BREAKPOINT_CONDITION_OR,
> 	PPC_BREAKPOINT_CONDITION_AND_OR, PPC_BREAKPOINT_CONDITION_BE_ALL,
> 	PPC_BREAKPOINT_CONDITION_BE_SHIFT, PPC_BREAKPOINT_CONDITION_BE):
> 	Define, in case <ptrace.h> doesn't provide it.
> 	(have_ptrace_new_debug_booke): New flag.
> 	(ppc_linux_check_watch_resources): Renamed to ...
> 	(ppc_linux_can_use_hw_breakpoint): ... this.  Handle BookE processors.
> 	(booke_debug_info): New variable.
> 	(max_slots_number): Ditto.
> 	(struct hw_break_tuple): New struct.
> 	(struct thread_points): Ditto.
> 	(ppc_threads): New variable.
> 	(PPC_DEBUG_CURRENT_VERSION): New define.
> 	(ppc_linux_region_ok_for_hw_watchpoint): Handle BookE processors.
> 	(booke_cmp_hw_point): New function.
> 	(booke_find_thread_points_by_tid): Ditto.
> 	(booke_insert_point): Ditto.
> 	(booke_remove_point): Ditto.
> 	(ppc_linux_insert_hw_breakpoint): Ditto.
> 	(ppc_linux_remove_hw_breakpoint): Ditto.
> 	(HW_WATCH_RW_TRIGGER): New define.
> 	(ppc_linux_insert_watchpoint): Handle BookE processors.
> 	(ppc_linux_remove_watchpoint): Ditto.
> 	(ppc_linux_new_thread): Ditto.
> 	(ppc_linux_stopped_data_address): Ditto.
> 	(ppc_linux_watchpoint_addr_within_range): Ditto.
> 	with and without the new kernel interface.
> 	(ppc_linux_read_description): Query the target to know if it
> 	supports the new kernel BookE interface.
> 	(_initialize_ppc_linux_nat): Initialize to_insert_hw_breakpoint and
> 	to_remove_hw_breakpoint fields of the target operations struct.

Overall, this looks OK to me.  A few general, non-blocking comments:

  (1) That's a lot of definitions to add at the beginning of the file,
      and I was going to suggest that we move them to a separate include.
      But I see that we've already started doing that with other
      definitions, so this is fine.

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

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.  The comments
above can be discussed as followup patches if you like.

> +/*
> + * 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):

/* 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 :).

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

> +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;
};

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

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

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

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

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

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

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

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

Same for ppc_linux_remove_watchpoint.

-- 
Joel