public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Luis Machado <lgustavo@codesourcery.com>
To: Pedro Alves <palves@redhat.com>, <gdb-patches@sourceware.org>,
	"Maciej W. Rozycki" <macro@imgtec.com>
Subject: Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values
Date: Wed, 24 Feb 2016 18:51:00 -0000	[thread overview]
Message-ID: <56CDFBC1.3000500@codesourcery.com> (raw)
In-Reply-To: <1456332239-24007-1-git-send-email-palves@redhat.com>

On 02/24/2016 01:43 PM, Pedro Alves wrote:
> This unbreaks pending/delayed breakpoints handling, as well as
> hardware watchpoints, on MIPS.
>
> Ref: https://sourceware.org/ml/gdb-patches/2016-02/msg00681.html
>
> The MIPS kernel reports SI_KERNEL for all kernel generated traps,
> instead of TRAP_BRKPT / TRAP_HWBKPT, but GDB isn't aware of this.
>
> Basically, this commit:
>
> - Folds watchpoints logic into check_stopped_by_breakpoint, and
>    renames it to save_stop_reason.
>
> - Adds GDB_ARCH_IS_TRAP_HWBKPT.
>
> - Makes MIPS set both GDB_ARCH_IS_TRAP_BRPT and
>    GDB_ARCH_IS_TRAP_HWBKPT to SI_KERNEL.  In save_stop_reason, we
>    handle the case of the same si_code returning true for both
>    TRAP_BRPT and TRAP_HWBKPT by looking at what the debug registers
>    say.
>
> Tested on x86-64 Fedora 20, native and gdbserver.
>
> gdb/ChangeLog:
> 2016-02-24  Pedro Alves  <palves@redhat.com>
>
> 	* linux-nat.c (save_sigtrap) Delete.
> 	(stop_wait_callback): Call save_stop_reason instead of
> 	save_sigtrap.
> 	(check_stopped_by_breakpoint): Rename to ...
> 	(save_stop_reason): ... this.  Bits of save_sigtrap folded here.
> 	Use GDB_ARCH_IS_TRAP_HWBKPT and handle ambiguous
> 	GDB_ARCH_IS_TRAP_BRKPT / GDB_ARCH_IS_TRAP_HWBKPT.  Factor out
> 	common code between the USE_SIGTRAP_SIGINFO and
> 	!USE_SIGTRAP_SIGINFO blocks.
> 	(linux_nat_filter_event): Call save_stop_reason instead of
> 	save_sigtrap.
> 	* nat/linux-ptrace.h: Check for both SI_KERNEL and TRAP_BRKPT
> 	si_code for MIPS.
> 	* nat/linux-ptrace.h: Fix "TRAP_HWBPT" typo in x86 table.  Add
> 	comments on MIPS behavior.
> 	(GDB_ARCH_IS_TRAP_HWBKPT): Define for all archs.
>
> gdb/gdbserver/ChangeLog:
> 2016-02-24  Pedro Alves  <palves@redhat.com>
>
> 	* linux-low.c (check_stopped_by_breakpoint): Rename to ...
> 	(save_stop_reason): ... this.  Use GDB_ARCH_IS_TRAP_HWBKPT and
> 	handle ambiguous GDB_ARCH_IS_TRAP_BRKPT / GDB_ARCH_IS_TRAP_HWBKPT.
> 	Factor out common code between the USE_SIGTRAP_SIGINFO and
> 	!USE_SIGTRAP_SIGINFO blocks.
> 	(linux_low_filter_event): Call save_stop_reason instead of
> 	check_stopped_by_breakpoint and check_stopped_by_watchpoint.
> 	Update comments.
> 	(linux_wait_1): Update comments.
> ---
>   gdb/gdbserver/linux-low.c | 170 ++++++++++++++++++++--------------------------
>   gdb/linux-nat.c           | 149 ++++++++++++++++++++--------------------
>   gdb/nat/linux-ptrace.h    |  38 ++++++++---
>   3 files changed, 172 insertions(+), 185 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 8b025bd..b02ad31 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -735,32 +735,16 @@ get_syscall_trapinfo (struct lwp_info *lwp, int *sysno, int *sysret)
>     current_thread = saved_thread;
>   }
>
> -/* This function should only be called if LWP got a SIGTRAP.
> -   The SIGTRAP could mean several things.
> +static int check_stopped_by_watchpoint (struct lwp_info *child);
>
> -   On i386, where decr_pc_after_break is non-zero:
> -
> -   If we were single-stepping this process using PTRACE_SINGLESTEP, we
> -   will get only the one SIGTRAP.  The value of $eip will be the next
> -   instruction.  If the instruction we stepped over was a breakpoint,
> -   we need to decrement the PC.
> -
> -   If we continue the process using PTRACE_CONT, we will get a
> -   SIGTRAP when we hit a breakpoint.  The value of $eip will be
> -   the instruction after the breakpoint (i.e. needs to be
> -   decremented).  If we report the SIGTRAP to GDB, we must also
> -   report the undecremented PC.  If the breakpoint is removed, we
> -   must resume at the decremented PC.
> -
> -   On a non-decr_pc_after_break machine with hardware or kernel
> -   single-step:
> -
> -   If we either single-step a breakpoint instruction, or continue and
> -   hit a breakpoint instruction, our PC will point at the breakpoint
> -   instruction.  */
> +/* Called when the LWP stopped for a signal/trap.  If it stopped for a
> +   trap check what caused it (breakpoint, watchpoint, trace, etc.),
> +   and save the result in the LWP's stop_reason field.  If it stopped
> +   for a breakpoint, decrement the PC if necessary on the lwp's
> +   architecture.  Returns true if we now have the LWP's stop PC.  */
>
>   static int
> -check_stopped_by_breakpoint (struct lwp_info *lwp)
> +save_stop_reason (struct lwp_info *lwp)
>   {
>     CORE_ADDR pc;
>     CORE_ADDR sw_breakpoint_pc;
> @@ -785,56 +769,39 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
>       {
>         if (siginfo.si_signo == SIGTRAP)
>   	{
> -	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
> +	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
> +	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
>   	    {
> -	      if (debug_threads)
> -		{
> -		  struct thread_info *thr = get_lwp_thread (lwp);
> -
> -		  debug_printf ("CSBB: %s stopped by software breakpoint\n",
> -				target_pid_to_str (ptid_of (thr)));
> -		}
> -
> -	      /* Back up the PC if necessary.  */
> -	      if (pc != sw_breakpoint_pc)
> -		{
> -		  struct regcache *regcache
> -		    = get_thread_regcache (current_thread, 1);
> -		  (*the_low_target.set_pc) (regcache, sw_breakpoint_pc);
> -		}
> -
> -	      lwp->stop_pc = sw_breakpoint_pc;
> +	      /* The si_code is ambiguous on this arch -- check debug
> +		 registers.  */
> +	      if (!check_stopped_by_watchpoint (lwp))
> +		lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> +	    }
> +	  else if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
> +	    {
> +	      /* If we determine the LWP stopped for a SW breakpoint,
> +		 trust it.  Particularly don't check watchpoint
> +		 registers, because at least on s390, we'd find
> +		 stopped-by-watchpoint as long as there's a watchpoint
> +		 set.  */
>   	      lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> -	      current_thread = saved_thread;
> -	      return 1;
>   	    }
> -	  else if (siginfo.si_code == TRAP_HWBKPT)
> +	  else if (GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
>   	    {
> -	      if (debug_threads)
> -		{
> -		  struct thread_info *thr = get_lwp_thread (lwp);
> -
> -		  debug_printf ("CSBB: %s stopped by hardware "
> -				"breakpoint/watchpoint\n",
> -				target_pid_to_str (ptid_of (thr)));
> -		}
> -
> -	      lwp->stop_pc = pc;
> -	      lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> -	      current_thread = saved_thread;
> -	      return 1;
> +	      /* This can indicate either a hardware breakpoint or
> +		 hardware watchpoint.  Check debug registers.  */
> +	      if (!check_stopped_by_watchpoint (lwp))
> +		lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
>   	    }
>   	  else if (siginfo.si_code == TRAP_TRACE)
>   	    {
> -	      if (debug_threads)
> -		{
> -		  struct thread_info *thr = get_lwp_thread (lwp);
> -
> -		  debug_printf ("CSBB: %s stopped by trace\n",
> -				target_pid_to_str (ptid_of (thr)));
> -		}
> -
> -	      lwp->stop_reason = TARGET_STOPPED_BY_SINGLE_STEP;
> +	      /* We may have single stepped an instruction that
> +		 triggered a watchpoint.  In that case, on some
> +		 architectures (such as x86), instead of TRAP_HWBKPT,
> +		 si_code indicates TRAP_TRACE, and we need to check
> +		 the debug registers separately.  */
> +	      if (!check_stopped_by_watchpoint (lwp))
> +		lwp->stop_reason = TARGET_STOPPED_BY_SINGLE_STEP;
>   	    }
>   	}
>       }
> @@ -845,6 +812,16 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
>        case we need to report the breakpoint PC.  */
>     if ((!lwp->stepping || lwp->stop_pc == sw_breakpoint_pc)
>         && (*the_low_target.breakpoint_at) (sw_breakpoint_pc))
> +    lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> +
> +  if (hardware_breakpoint_inserted_here (pc))
> +    lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> +
> +  if (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON)
> +    check_stopped_by_watchpoint (lp);
> +#endif
> +
> +  if (lwp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
>       {
>         if (debug_threads)
>   	{
> @@ -856,19 +833,16 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
>
>         /* Back up the PC if necessary.  */
>         if (pc != sw_breakpoint_pc)
> -        {
> +	{
>   	  struct regcache *regcache
>   	    = get_thread_regcache (current_thread, 1);
>   	  (*the_low_target.set_pc) (regcache, sw_breakpoint_pc);
>   	}
>
> -      lwp->stop_pc = sw_breakpoint_pc;
> -      lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> -      current_thread = saved_thread;
> -      return 1;
> +      /* Update this so we record the correct stop PC below.  */
> +      pc = sw_breakpoint_pc;
>       }
> -
> -  if (hardware_breakpoint_inserted_here (pc))
> +  else if (lwp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT)
>       {
>         if (debug_threads)
>   	{
> @@ -877,16 +851,31 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
>   	  debug_printf ("CSBB: %s stopped by hardware breakpoint\n",
>   			target_pid_to_str (ptid_of (thr)));
>   	}
> +    }
> +  else if (lwp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT)
> +    {
> +      if (debug_threads)
> +	{
> +	  struct thread_info *thr = get_lwp_thread (lwp);
>
> -      lwp->stop_pc = pc;
> -      lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> -      current_thread = saved_thread;
> -      return 1;
> +	  debug_printf ("CSBB: %s stopped by hardware watchpoint\n",
> +			target_pid_to_str (ptid_of (thr)));
> +	}
>       }
> -#endif
> +  else if (lwp->stop_reason == TARGET_STOPPED_BY_SINGLE_STEP)
> +    {
> +      if (debug_threads)
> +	{
> +	  struct thread_info *thr = get_lwp_thread (lwp);
>
> +	  debug_printf ("CSBB: %s stopped by trace\n",
> +			target_pid_to_str (ptid_of (thr)));
> +	}
> +    }
> +
> +  lwp->stop_pc = pc;
>     current_thread = saved_thread;
> -  return 0;
> +  return 1;
>   }
>
>   static struct lwp_info *
> @@ -2434,8 +2423,8 @@ linux_low_filter_event (int lwpid, int wstat)
>         child->syscall_state = TARGET_WAITKIND_IGNORE;
>       }
>
> -  /* Be careful to not overwrite stop_pc until
> -     check_stopped_by_breakpoint is called.  */
> +  /* Be careful to not overwrite stop_pc until save_stop_reason is
> +     called.  */
>     if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGTRAP
>         && linux_is_extended_waitstatus (wstat))
>       {
> @@ -2448,27 +2437,12 @@ linux_low_filter_event (int lwpid, int wstat)
>   	}
>       }
>
> -  /* Check first whether this was a SW/HW breakpoint before checking
> -     watchpoints, because at least s390 can't tell the data address of
> -     hardware watchpoint hits, and returns stopped-by-watchpoint as
> -     long as there's a watchpoint set.  */
>     if (WIFSTOPPED (wstat) && linux_wstatus_maybe_breakpoint (wstat))
>       {
> -      if (check_stopped_by_breakpoint (child))
> +      if (save_stop_reason (child))
>   	have_stop_pc = 1;
>       }
>
> -  /* Note that TRAP_HWBKPT can indicate either a hardware breakpoint
> -     or hardware watchpoint.  Check which is which if we got
> -     TARGET_STOPPED_BY_HW_BREAKPOINT.  Likewise, we may have single
> -     stepped an instruction that triggered a watchpoint.  In that
> -     case, on some architectures (such as x86), instead of
> -     TRAP_HWBKPT, si_code indicates TRAP_TRACE, and we need to check
> -     the debug registers separately.  */
> -  if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGTRAP
> -      && child->stop_reason != TARGET_STOPPED_BY_SW_BREAKPOINT)
> -    check_stopped_by_watchpoint (child);
> -
>     if (!have_stop_pc)
>       child->stop_pc = get_pc (child);
>
> @@ -3209,7 +3183,7 @@ linux_wait_1 (ptid_t ptid,
>        hardware single step it means a gdb/gdbserver breakpoint had been
>        planted on top of a permanent breakpoint, in the case of a software
>        single step it may just mean that gdbserver hit the reinsert breakpoint.
> -     The PC has been adjusted by check_stopped_by_breakpoint to point at
> +     The PC has been adjusted by save_stop_reason to point at
>        the breakpoint address.
>        So in the case of the hardware single step advance the PC manually
>        past the breakpoint and in the case of software single step advance only
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 6ded38d..0829bcb 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -303,10 +303,11 @@ static struct lwp_info *find_lwp_pid (ptid_t ptid);
>
>   static int lwp_status_pending_p (struct lwp_info *lp);
>
> -static int check_stopped_by_breakpoint (struct lwp_info *lp);
>   static int sigtrap_is_event (int status);
>   static int (*linux_nat_status_is_event) (int status) = sigtrap_is_event;
>
> +static void save_stop_reason (struct lwp_info *lp);
> +
>   \f
>   /* LWP accessors.  */
>
> @@ -2321,30 +2322,6 @@ check_stopped_by_watchpoint (struct lwp_info *lp)
>     return lp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT;
>   }
>
> -/* Called when the LWP stopped for a trap that could be explained by a
> -   watchpoint or a breakpoint.  */
> -
> -static void
> -save_sigtrap (struct lwp_info *lp)
> -{
> -  gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON);
> -  gdb_assert (lp->status != 0);
> -
> -  /* Check first if this was a SW/HW breakpoint before checking
> -     watchpoints, because at least s390 can't tell the data address of
> -     hardware watchpoint hits, and the kernel returns
> -     stopped-by-watchpoint as long as there's a watchpoint set.  */
> -  if (linux_nat_status_is_event (lp->status))
> -    check_stopped_by_breakpoint (lp);
> -
> -  /* Note that TRAP_HWBKPT can indicate either a hardware breakpoint
> -     or hardware watchpoint.  Check which is which if we got
> -     TARGET_STOPPED_BY_HW_BREAKPOINT.  */
> -  if (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON
> -      || lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT)
> -    check_stopped_by_watchpoint (lp);
> -}
> -
>   /* Returns true if the LWP had stopped for a watchpoint.  */
>
>   static int
> @@ -2441,7 +2418,7 @@ stop_wait_callback (struct lwp_info *lp, void *data)
>   	  /* Save the sigtrap event.  */
>   	  lp->status = status;
>   	  gdb_assert (lp->signalled);
> -	  save_sigtrap (lp);
> +	  save_stop_reason (lp);
>   	}
>         else
>   	{
> @@ -2583,29 +2560,32 @@ select_event_lwp_callback (struct lwp_info *lp, void *data)
>     return 0;
>   }
>
> -/* Called when the LWP got a signal/trap that could be explained by a
> -   software or hardware breakpoint.  */
> +/* Called when the LWP stopped for a signal/trap.  If it stopped for a
> +   trap check what caused it (breakpoint, watchpoint, trace, etc.),
> +   and save the result in the LWP's stop_reason field.  If it stopped
> +   for a breakpoint, decrement the PC if necessary on the lwp's
> +   architecture.  */
>
> -static int
> -check_stopped_by_breakpoint (struct lwp_info *lp)
> +static void
> +save_stop_reason (struct lwp_info *lp)
>   {
> -  /* Arrange for a breakpoint to be hit again later.  We don't keep
> -     the SIGTRAP status and don't forward the SIGTRAP signal to the
> -     LWP.  We will handle the current event, eventually we will resume
> -     this LWP, and this breakpoint will trap again.
> -
> -     If we do not do this, then we run the risk that the user will
> -     delete or disable the breakpoint, but the LWP will have already
> -     tripped on it.  */
> -
> -  struct regcache *regcache = get_thread_regcache (lp->ptid);
> -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +  struct regcache *regcache;
> +  struct gdbarch *gdbarch;
>     CORE_ADDR pc;
>     CORE_ADDR sw_bp_pc;
>   #if USE_SIGTRAP_SIGINFO
>     siginfo_t siginfo;
>   #endif
>
> +  gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON);
> +  gdb_assert (lp->status != 0);
> +
> +  if (!linux_nat_status_is_event (lp->status))
> +    return;
> +
> +  regcache = get_thread_regcache (lp->ptid);
> +  gdbarch = get_regcache_arch (regcache);
> +
>     pc = regcache_read_pc (regcache);
>     sw_bp_pc = pc - gdbarch_decr_pc_after_break (gdbarch);
>
> @@ -2614,33 +2594,29 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
>       {
>         if (siginfo.si_signo == SIGTRAP)
>   	{
> -	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
> +	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
> +	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
>   	    {
> -	      if (debug_linux_nat)
> -		fprintf_unfiltered (gdb_stdlog,
> -				    "CSBB: %s stopped by software "
> -				    "breakpoint\n",
> -				    target_pid_to_str (lp->ptid));
> -
> -	      /* Back up the PC if necessary.  */
> -	      if (pc != sw_bp_pc)
> -		regcache_write_pc (regcache, sw_bp_pc);
> -
> -	      lp->stop_pc = sw_bp_pc;
> +	      /* The si_code is ambiguous on this arch -- check debug
> +		 registers.  */
> +	      if (!check_stopped_by_watchpoint (lp))
> +		lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> +	    }
> +	  else if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
> +	    {
> +	      /* If we determine the LWP stopped for a SW breakpoint,
> +		 trust it.  Particularly don't check watchpoint
> +		 registers, because at least on s390, we'd find
> +		 stopped-by-watchpoint as long as there's a watchpoint
> +		 set.  */
>   	      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> -	      return 1;
>   	    }
> -	  else if (siginfo.si_code == TRAP_HWBKPT)
> +	  else if (GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
>   	    {
> -	      if (debug_linux_nat)
> -		fprintf_unfiltered (gdb_stdlog,
> -				    "CSBB: %s stopped by hardware "
> -				    "breakpoint/watchpoint\n",
> -				    target_pid_to_str (lp->ptid));
> -
> -	      lp->stop_pc = pc;
> -	      lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> -	      return 1;
> +	      /* This can indicate either a hardware breakpoint or
> +		 hardware watchpoint.  Check debug registers.  */
> +	      if (!check_stopped_by_watchpoint (lp))
> +		lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
>   	    }
>   	  else if (siginfo.si_code == TRAP_TRACE)
>   	    {
> @@ -2648,6 +2624,13 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
>   		fprintf_unfiltered (gdb_stdlog,
>   				    "CSBB: %s stopped by trace\n",
>   				    target_pid_to_str (lp->ptid));
> +
> +	      /* We may have single stepped an instruction that
> +		 triggered a watchpoint.  In that case, on some
> +		 architectures (such as x86), instead of TRAP_HWBKPT,
> +		 si_code indicates TRAP_TRACE, and we need to check
> +		 the debug registers separately.  */
> +	      check_stopped_by_watchpoint (lp);
>   	    }
>   	}
>       }
> @@ -2658,6 +2641,18 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
>       {
>         /* The LWP was either continued, or stepped a software
>   	 breakpoint instruction.  */
> +      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> +    }
> +
> +  if (hardware_breakpoint_inserted_here_p (get_regcache_aspace (regcache), pc))
> +    lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> +
> +  if (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON)
> +    check_stopped_by_watchpoint (lp);
> +#endif
> +
> +  if (lp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
> +    {
>         if (debug_linux_nat)
>   	fprintf_unfiltered (gdb_stdlog,
>   			    "CSBB: %s stopped by software breakpoint\n",
> @@ -2667,25 +2662,25 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
>         if (pc != sw_bp_pc)
>   	regcache_write_pc (regcache, sw_bp_pc);
>
> -      lp->stop_pc = sw_bp_pc;
> -      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> -      return 1;
> +      /* Update this so we record the correct stop PC below.  */
> +      pc = sw_bp_pc;
>       }
> -
> -  if (hardware_breakpoint_inserted_here_p (get_regcache_aspace (regcache), pc))
> +  else if (lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT)
>       {
>         if (debug_linux_nat)
>   	fprintf_unfiltered (gdb_stdlog,
> -			    "CSBB: stopped by hardware breakpoint %s\n",
> +			    "CSBB: %s stopped by hardware breakpoint\n",
> +			    target_pid_to_str (lp->ptid));
> +    }
> +  else if (lp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT)
> +    {
> +      if (debug_linux_nat)
> +	fprintf_unfiltered (gdb_stdlog,
> +			    "CSBB: %s stopped by hardware watchpoint\n",
>   			    target_pid_to_str (lp->ptid));
> -
> -      lp->stop_pc = pc;
> -      lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> -      return 1;
>       }
> -#endif
>
> -  return 0;
> +  lp->stop_pc = pc;
>   }
>
>
> @@ -3057,7 +3052,7 @@ linux_nat_filter_event (int lwpid, int status)
>     /* An interesting event.  */
>     gdb_assert (lp);
>     lp->status = status;
> -  save_sigtrap (lp);
> +  save_stop_reason (lp);
>     return lp;
>   }
>
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index ba58717..1d137e8 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -119,14 +119,14 @@ struct buffer;
>   /* The x86 kernel gets some of the si_code values backwards, like
>      this:
>
> -   | what                                     | si_code    |
> -   |------------------------------------------+------------|
> -   | software breakpoints (int3)              | SI_KERNEL  |
> -   | single-steps                             | TRAP_TRACE |
> -   | single-stepping a syscall                | TRAP_BRKPT |
> -   | user sent SIGTRAP                        | 0          |
> -   | exec SIGTRAP (when no PTRACE_EVENT_EXEC) | 0          |
> -   | hardware breakpoints/watchpoints         | TRAP_HWBPT |
> +   | what                                     | si_code     |
> +   |------------------------------------------+-------------|
> +   | software breakpoints (int3)              | SI_KERNEL   |
> +   | single-steps                             | TRAP_TRACE  |
> +   | single-stepping a syscall                | TRAP_BRKPT  |
> +   | user sent SIGTRAP                        | 0           |
> +   | exec SIGTRAP (when no PTRACE_EVENT_EXEC) | 0           |
> +   | hardware breakpoints/watchpoints         | TRAP_HWBKPT |
>
>      That is, it reports SI_KERNEL for software breakpoints (and only
>      for those), and TRAP_BRKPT for single-stepping a syscall...  If the
> @@ -140,14 +140,32 @@ struct buffer;
>      in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
>      on a SIGTRAP for any other reason.
>
> -   The generic Linux target code should use GDB_ARCH_IS_TRAP_BRKPT
> -   instead of TRAP_BRKPT to abstract out these peculiarities.  */
> +   The MIPS kernel uses SI_KERNEL for all kernel generated traps.
> +   Since:
> +
> +     - MIPS doesn't do hardware single-step
> +     - We don't need to care about exec SIGTRAPs, since we assume
> +       PTRACE_EVENT_EXEC.
> +     - The MIPS kernel doesn't support hardware breakpoints.
> +
> +   on MIPS, all we need to care about is distinguishing between
> +   software breakpoints and hardware watchpoints, which can be done by
> +   peeking the debug registers.
> +
> +   The generic Linux target code should use GDB_ARCH_IS_TRAP_* instead
> +   of TRAP_* to abstract out these peculiarities.  */
>   #if defined __i386__ || defined __x86_64__
>   # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
>   #elif defined __powerpc__
>   # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
> +#elif defined __mips__
> +# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL)
>   #else
>   # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == TRAP_BRKPT)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
>   #endif
>
>   #ifndef TRAP_HWBKPT
>

I've tried it with a few MIPS/Linux boards and it works fine for 
hardware watchpoints and software breakpoints again.

Thanks,
Luis

      parent reply	other threads:[~2016-02-24 18:51 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-24 16:44 Pedro Alves
2016-02-24 18:29 ` Maciej W. Rozycki
2016-02-24 18:51   ` Pedro Alves
2016-02-24 19:02     ` Luis Machado
2016-02-24 20:49       ` Maciej W. Rozycki
2016-02-24 21:10         ` Luis Machado
2016-02-24 20:39     ` Maciej W. Rozycki
2016-02-24 23:20       ` Pedro Alves
2016-02-24 23:37         ` [PATCH] MIPS/Linux: Also recognize TRAP_BRKPT and TRAP_HWBKPT Pedro Alves
2016-02-25  1:39         ` [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values Maciej W. Rozycki
2016-02-25  1:45           ` Pedro Alves
2016-04-05 16:57             ` Maciej W. Rozycki
2016-04-15 23:17               ` Pedro Alves
2016-02-24 18:51 ` Luis Machado [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56CDFBC1.3000500@codesourcery.com \
    --to=lgustavo@codesourcery.com \
    --cc=gdb-patches@sourceware.org \
    --cc=macro@imgtec.com \
    --cc=palves@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).