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

Ulrich Weigand
2019-08-08T16:24:36Z

> gdb/ChangeLog:
> YYYY-MM-DD Pedro Franco de Carvalho <pedromfc@linux.ibm.com>
>
> * ppc-linux-nat.c: Include <algorithm>, <unordered_map>, and
> .
> (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,