Re: [PATCH 3/3] [PowerPC] Fix debug register issues in ppc-linux-nat

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

Pedro Franco de Carvalho wrote:

> gdb/ChangeLog:
> YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>
> 
> 	* ppc-linux-nat.c: Include <algorithm>, <unordered_map>, and
> 	<list>.
> 	(PPC_DEBUG_CURRENT_VERSION): Move up define.
> 	(struct arch_lwp_info): New struct.
> 	(class ppc_linux_dreg_interface): New class.
> 	(struct ppc_linux_nat_target) <thread_exit_cb, low_delete_thread>
> 	<low_new_fork, low_new_clone, low_forget_process>
> 	<low_prepare_to_resume, copy_thread_dreg_state>
> 	<mark_thread_stale, mark_debug_registers_changed>
> 	<register_hw_breakpoint, clear_hw_breakpoint, register_wp>
> 	<clear_wp, can_use_watchpoint_cond_accel, calculate_dvc>
> 	<check_condition, num_memory_accesses, get_trigger_type>
> 	<create_watchpoint_request, hwdebug_point_cmp>
> 	<get_arch_lwp_info>: Declare as methods.
> 	<struct ptid_hash>: New inner struct.
> 	<m_dreg_interface, m_requested_hw_bps, m_requested_wp_vals>
> 	<m_installed_hw_bps>: Declare members.
> 	(saved_dabr_value, hwdebug_info, max_slots_number)
> 	(struct hw_break_tuple, struct thread_points, ppc_threads)
> 	(have_ptrace_hwdebug_interface)
> 	(hwdebug_find_thread_points_by_tid)
> 	(hwdebug_insert_point, hwdebug_remove_point): Remove.
> 	(ppc_linux_nat_target::can_use_hw_breakpoint): Use
> 	m_dreg_interface, remove call to PTRACE_SET_DEBUGREG.
> 	(ppc_linux_nat_target::region_ok_for_hw_watchpoint): Add comment,
> 	use m_dreg_interface.
> 	(hwdebug_point_cmp): Change to...
> 	(ppc_linux_nat_target::hwdebug_point_cmp): ...this method.  Use
> 	reference arguments instead of pointers.
> 	(ppc_linux_nat_target::ranged_break_num_registers): Use
> 	m_dreg_interface.
> 	(ppc_linux_nat_target::insert_hw_breakpoint): Add comment, use
> 	m_dreg_interface.  Call register_hw_breakpoint.
> 	(ppc_linux_nat_target::remove_hw_breakpoint): Add comment, use
> 	m_dreg_interface.  Call clear_hw_breakpoint.
> 	(get_trigger_type): Change to...
> 	(ppc_linux_nat_target::get_trigger_type): ...this method.  Add
> 	comment.
> 	(ppc_linux_nat_target::insert_mask_watchpoint): Update comment,
> 	use m_dreg_interface.  Call register_hw_breakpoint.
> 	(ppc_linux_nat_target::remove_mask_watchpoint): Update comment,
> 	use m_dreg_interface.  Call clear_hw_breakpoint.
> 	(can_use_watchpoint_cond_accel): Change to...
> 	(ppc_linux_nat_target::can_use_watchpoint_cond_accel): ...this
> 	method.  Update comment, use m_dreg_interface and
> 	m_process_hw_breakpoints.
> 	(calculate_dvc): Change to...
> 	(ppc_linux_nat_target::calculate_dvc): ...this method.  Use
> 	m_dreg_interface.
> 	(num_memory_accesses): Change to...
> 	(ppc_linux_nat_target::num_memory_accesses): ...this method.
> 	(check_condition): Change to...
> 	(ppc_linux_nat_target::check_condition): ...this method.
> 	(ppc_linux_nat_target::can_accel_watchpoint_condition): Update
> 	comment, use m_dreg_interface.
> 	(create_watchpoint_request): Change to...
> 	(ppc_linux_nat_target::create_watchpoint_request): ...this
> 	method.  Use m_dreg_interface.
> 	(ppc_linux_nat_target::insert_watchpoint): Add comment, use
> 	m_dreg_interface.  Call register_hw_breakpoint or register_wp.
> 	(ppc_linux_nat_target::remove_watchpoint): Add comment, use
> 	m_dreg_interface.  Call clear_hw_breakpoint or clear_wp.
> 	(ppc_linux_nat_target::low_forget_process)
> 	(ppc_linux_nat_target::low_new_fork)
> 	(ppc_linux_nat_target::low_new_clone)
> 	(ppc_linux_nat_target::low_delete_thread)
> 	(ppc_linux_nat_target::low_prepare_to_resume): New methods.
> 	(ppc_linux_nat_target::low_new_thread): Remove previous logic,
> 	only call mark_thread_stale.
> 	(ppc_linux_nat_target::thread_exit_cb): New method.
> 	(ppc_linux_thread_exit): Remove.
> 	(ppc_linux_nat_target::stopped_data_address): Add comment, use
> 	m_dreg_interface and m_thread_hw_breakpoints.
> 	(ppc_linux_nat_target::stopped_by_watchpoint): Add comment.
> 	(ppc_linux_nat_target::watchpoint_addr_within_range): Use
> 	m_dreg_interface.
> 	(ppc_linux_nat_target::masked_watch_num_registers): Use
> 	m_dreg_interface.
> 	(ppc_linux_nat_target::copy_thread_dreg_state)
> 	(ppc_linux_nat_target::mark_thread_stale)
> 	(ppc_linux_nat_target::mark_debug_registers_changed)
> 	(ppc_linux_nat_target::register_hw_breakpoint)
> 	(ppc_linux_nat_target::clear_hw_breakpoint)
> 	(ppc_linux_nat_target::register_wp)
> 	(ppc_linux_nat_target::clear_wp)
> 	(ppc_linux_nat_target::get_arch_lwp_info): New methods.
> 	(_initialize_ppc_linux_nat): Change the callback function for the
> 	thread_exit observable.

This looks generally good to me, just two questions:

- As mentioned in the 1/3 patch, why do you need the low_new_clone
  callback?  As I understand it, you'll get low_new_thread called
  immediatedly afterwards, which will mark the thread as "stale",
  and once it is scheduled again, all debug regs will be set up
  from scratch anyway ...

- We currently do not support hardware watchpoints in gdbserver,
  even though we really should.  Ideally, the low-level code to
  handle debug regs should be shared between gdb and gdbserver,
  as is done e.g. on x86.  Now, I'm not saying that handling
  gdbserver is a pre-req for this patch (fixing GDB first is of
  course fine!), but I'm wondering if it would make sense, given
  that you're refactoring a lot of this code anyway, to think
  about whether this setup would help or hinder a future merge
  with gdbserver.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com