This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Stale breakpoint instructions, spurious SIGTRAPS.


On 04/16/2014 02:18 AM, Pedro Alves wrote:
> ppc_linux_memory_remove_breakpoint
> does this unconditionally for all memory breakpoints, and questions
> whether memory_remove_breakpoint should be changed to do this for all
> breakpoints.  Possibly yes, though I'm not certain, hence this
> baby-steps patch.
> 
> Comments?

As it is said in the comments of ppc_linux_memory_remove_breakpoint,
"more traffic is generated for the remote targets".  I'd like not to
"validate memory before writing shadow contents back" in
memory_remove_breakpoint unless we have more evidence that it is useful
to more targets or scenarios.

This patch looks good to me, some comments below.


> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index f777a4a..8c14bb1 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2648,7 +2648,9 @@ insert_bp_location (struct bp_location *bl,
>  	     target doesn't define error codes), so we must treat
>  	     generic errors as memory errors.  */
>  	  if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR)
> -	      && solib_name_from_address (bl->pspace, bl->address))
> +	      && (solib_name_from_address (bl->pspace, bl->address)
> +		  || userloaded_objfile_contains_address_p (bl->pspace,
> +							    bl->address)))
>  	    {
>  	      /* See also: disable_breakpoints_in_shlibs.  */
>  	      bl->shlib_disabled = 1;
> @@ -3778,7 +3780,27 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
>  	  || !(section_is_overlay (bl->section)))
>  	{
>  	  /* No overlay handling: just remove the breakpoint.  */
> -	  val = bl->owner->ops->remove_location (bl);
> +
> +	  /* If we're trying to uninsert a memory breakpoint that we
> +	     know is set in a dynamic object that is marked
> +	     shlib_disabled, then either the dynamic object was
> +	     removed with remove-symbol-file or with
> +	     "nosharedlibrary".  In the former case, we don't know
> +	     whether another dynamic object might have loaded over the
> +	     breakpoint's address -- the user might well let us know
> +	     about it next with add-symbol-file (the whole point of
> +	     OBJF_USERLOADED is letting the user manually maintain a
> +	     list of dynamically loaded objects).  Check if the
> +	     breakpoint is still inserted in memory, to avoid
> +	     overwriting wrong code with stale saved shadow contents.
> +	     HW breakpoints are independent of poking at memory and
> +	     must always be removed.  */
> +	  if (bl->shlib_disabled
> +	      && bl->target_info.shadow_len != 0

Does this condition mean "BL is software breakpoint location"?  If so,
we can comment this explicitly and move comment "HW breakpoints are ..."
to this line above.  Otherwise, people (at least I) may ask why don't
check "bl->loc_type == bp_loc_software_breakpoint).

> +	      && !memory_validate_breakpoint (bl->gdbarch, &bl->target_info))
> +	    val = 0;
> +	  else
> +	    val = bl->owner->ops->remove_location (bl);
>  	}
>        else
>  	{
> @@ -3823,10 +3845,15 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
>  	    }
>  	}
>  
> -      /* In some cases, we might not be able to remove a breakpoint
> -	 in a shared library that has already been removed, but we
> -	 have not yet processed the shlib unload event.  */
> -      if (val && solib_name_from_address (bl->pspace, bl->address))
> +      /* In some cases, we might not be able to remove a breakpoint in
> +	 a shared library that has already been removed, but we have
> +	 not yet processed the shlib unload event.  Similarly for an
> +	 unloaded add-symbol-file object - the user might not yet have
> +	 had the chance to remove-symbol-file it.  */
> +      if (val && (bl->shlib_disabled
> +		  || solib_name_from_address (bl->pspace, bl->address)
> +		  || userloaded_objfile_contains_address_p (bl->pspace,
> +							    bl->address)))
>  	val = 0;

I can understand why userloaded_objfile_contains_address_p is added
here, but check to "bl->shlib_disabled" isn't explained in the comments
or changelog entry.

> diff --git a/gdb/mem-break.c b/gdb/mem-break.c
> index 1a057df..5371bd0 100644
> --- a/gdb/mem-break.c
> +++ b/gdb/mem-break.c
> @@ -89,3 +89,36 @@ memory_remove_breakpoint (struct target_ops *ops, struct gdbarch *gdbarch,
>  {
>    return gdbarch_memory_remove_breakpoint (gdbarch, bp_tgt);
>  }
> +
> +int
> +memory_validate_breakpoint (struct gdbarch *gdbarch,
> +			    struct bp_target_info *bp_tgt)
> +{
> +  CORE_ADDR addr = bp_tgt->placed_address;
> +  const gdb_byte *bp;
> +  int val;
> +  int bplen;
> +  gdb_byte old_contents[BREAKPOINT_MAX];
> +  struct cleanup *cleanup;
> +  int ret;
> +
> +  /* Determine appropriate breakpoint contents and size for this
> +     address.  */
> +  bp = gdbarch_breakpoint_from_pc (gdbarch, &addr, &bplen);
> +
> +  if (bp == NULL
> +      || bp_tgt->placed_size != bplen)

Nit: they may fit in one line?

> +
> +	# Now delete the breakpoint from GDB's tables, to make sure
> +	# GDB doesn't reinsert it, masking the bug (with the bug, on
> +	# re-insert, GDB would fill the shadow buffer with a
> +	# breakpoint instruction).
> +	set test "delete"
> +	gdb_test_multiple $test $test {
> +	    -re ".*y or n. $" {
> +		send_gdb "y\n"
> +		exp_continue
> +	    }
> +	    -re "$gdb_prompt $" {
> +		pass $test
> +	    }
> +	}

Use delete_breakpoints? or something like
gdb_test "$test" "" "$test" "Delete all breakpoints.*y or n.*$" "y"

> +
> +	# Re-add symbols back.
> +	set test "file \$binfile"
> +	gdb_test_multiple "file $binfile" $test {
> +	    -re "Are you sure you want to change the file. .*y or n. $" {
> +		send_gdb "y\n"
> +		exp_continue
> +	    }
> +	    -re "Reading symbols from.*done.*$gdb_prompt $" {
> +		pass $test
> +	    }
> +	}
> +
> +	# Run to another function now.  With the bug, GDB would trip
> +	# on a spurious trap at foo.
> +	gdb_test "b bar" ".*reakpoint .* at .*"
> +	gdb_test "continue" "Breakpoint .*, bar .*"
> +    }
> +}
> +
> +# While it doesn't trigger the original bug this is a regression test
> +# for, test with breakpoint always-inserted off for extra coverage.
> +foreach always_inserted { "off" "on" } {
> +    foreach break_command { "break" "hbreak" } {
> +	test_break $always_inserted $break_command

We need to check skip_hw_breakpoint_tests and don't pass "hbreak" to
test_break if skip_hw_breakpoint_tests returns true.

-- 
Yao (éå)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]