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

["Ulrich Weigand" <uweigand@de.ibm.com>]

Joel Brobecker wrote:
> > 2009-12-31  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 global.
> > 	(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.
> > 	(hw_break_tuple): New 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.
> > 	(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.
> > 	(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.
> 
> Still looks good to me :). Just a couple of nits, but otherwise approved. 
> It would still be nice if Ulrich had a chance to take a quick look ;-).

Looks good to me as well.  The only nit I've found is this:

@@ -1595,6 +2012,20 @@ ppc_linux_read_description (struct target_ops *ops)
 	perror_with_name (_("Unable to fetch AltiVec registers"));
     }
 
+  /* Check for kernel support for new BOOKE debugging registers.  */
+  if (ptrace (PPC_PTRACE_GETHWDBGINFO, tid, 0, &booke_debug_info) >= 0)
+    {
+      have_ptrace_new_debug_booke = 1;
+      max_slots_number = booke_debug_info.num_instruction_bps
+	+ booke_debug_info.num_data_bps + booke_debug_info.num_condition_regs;
+    }
+  else
+    {
+      /* Old school interface and no new BOOKE registers support.  */
+      have_ptrace_new_debug_booke = 0;
+      memset (&booke_debug_info, 0, sizeof (struct ppc_debug_info));
+    }
+
   /* Power ISA 2.05 (implemented by Power 6 and newer processors) increases
      the FPSCR from 32 bits to 64 bits. Even though Power 7 supports this
      ISA version, it doesn't have PPC_FEATURE_ARCH_2_05 set, only


Setting global variables like have_ptrace_new_debug_booke as an implicit
side effect of ppc_linux_read_description isn't really nice; it makes
assumptions about the sequence in which these routines are called that
may not be guaranteed after potential future changes ...

Why not use a routine to retrieve the info that is called whereever
necessary (the routine can internally cache the data and the fact
that it useless to try as kernel support is missing)?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com