[patch] Fix stale tp->step_resume_breakpoint

[patch] Fix stale tp->step_resume_breakpoint

[Jan Kratochvil]

Hi,

while debugging unrelated issue I got some:
	bpstat_what: unhandled bptype -3735941133 (garbage)

due to a stale memory below.

I did not create a special testcase but it is reproducible with valgrind and
the following code (which is not a minimal reproducer):

Invalid read of size 4
   at: is_hardware_watchpoint (breakpoint.c:1212)
   by: is_watchpoint (breakpoint.c:1223)
   by: bpstat_check_watchpoint (breakpoint.c:3741)
   by: bpstat_stop_status (breakpoint.c:4098)
   by: handle_inferior_event (infrun.c:3898)
   by: wait_for_inferior (infrun.c:2575)
   by: proceed (infrun.c:2106)
   by: step_once (infcmd.c:1031)
   by: step_1 (infcmd.c:880)
   by: stepi_command (infcmd.c:816)
   by: do_cfunc (cli-decode.c:67)
   by: cmd_func (cli-decode.c:1771)
   by: execute_command (top.c:422)
   by: command_loop (top.c:534)
   by: read_command_file (top.c:321)
   by: wrapped_read_command_file (cli-script.c:1592)
   by: catch_exception (exceptions.c:468)
   by: script_from_file (cli-script.c:1622)
   by: source_script_from_stream (cli-cmds.c:557)
   by: source_script_with_search (cli-cmds.c:588)
   by: source_script (cli-cmds.c:598)
   by: catch_command_errors (exceptions.c:534)
   by: captured_main (main.c:886)
   by: catch_errors (exceptions.c:518)
   by: gdb_main (main.c:921)
   by: main (gdb.c:34)
 Address 0xb62d808 is 8 bytes inside a block of size 368 free'd
   at: free (vg_replace_malloc.c:325)
   by: xfree (utils.c:1525)
   by: delete_breakpoint (breakpoint.c:9733)
   by: delete_step_resume_breakpoint (thread.c:88)
   by: delete_step_resume_breakpoint_callback (infrun.c:2339)
   by: iterate_over_threads (thread.c:353)
   by: delete_step_thread_step_resume_breakpoint (infrun.c:2368)
   by: delete_step_thread_step_resume_breakpoint_cleanup (infrun.c:2376)
   by: do_my_cleanups (utils.c:479)
   by: do_cleanups (utils.c:461)
   by: wait_for_inferior (infrun.c:2584)
   by: proceed (infrun.c:2106)
   by: run_inferior_call (infcall.c:378)
   by: call_function_by_hand (infcall.c:804)
   by: evaluate_subexp_standard (eval.c:1816)
   by: evaluate_subexp_c (c-lang.c:1047)
   by: evaluate_subexp (eval.c:76)
   by: evaluate_expression (eval.c:167)
   by: breakpoint_cond_eval (breakpoint.c:3431)
   by: catch_errors (exceptions.c:518)
   by: bpstat_check_breakpoint_conditions (breakpoint.c:3959)
   by: bpstat_stop_status (breakpoint.c:4110)
   by: handle_inferior_event (infrun.c:3898)
   by: wait_for_inferior (infrun.c:2575)
   by: proceed (infrun.c:2106)
   by: step_once (infcmd.c:1031)
   by: step_1 (infcmd.c:880)
   by: stepi_command (infcmd.c:816)
   by: do_cfunc (cli-decode.c:67)
   by: cmd_func (cli-decode.c:1771)
   by: execute_command (top.c:422)
   by: command_loop (top.c:534)
   by: read_command_file (top.c:321)
   by: wrapped_read_command_file (cli-script.c:1592)
   by: catch_exception (exceptions.c:468)
   by: script_from_file (cli-script.c:1622)
   by: source_script_from_stream (cli-cmds.c:557)
   by: source_script_with_search (cli-cmds.c:588)
   by: source_script (cli-cmds.c:598)
   by: catch_command_errors (exceptions.c:534)
   by: captured_main (main.c:886)
   by: catch_errors (exceptions.c:518)
   by: gdb_main (main.c:921)
   by: main (gdb.c:34)

#define _GNU_SOURCE
#include <stdio.h>
#include <signal.h>
#include <pthread.h>
#include <asm/unistd.h>
#include <unistd.h>
#define tgkill(pid, tid, sig) syscall (__NR_tgkill, (pid), (tid), (sig))
#define gettid() syscall (__NR_gettid)
int condA (void) { puts ("condA"); return 1; }
int condB (void) { puts ("condB"); return 1; }
__attribute__((noinline, noclone)) void loop (void)
{
  volatile int i = 0;
  for (;;);
    i = 0;
}
static void *start (void *arg)
{
  for (;;)
    {
      tgkill (getpid (), getpid (), SIGUSR1);
      usleep (10000);
    }
  return arg;
}
int main (void)
{
  pthread_t thread;
  pthread_create (&thread, NULL, start, NULL);
  loop ();
  return 0;
}

file ./1
handle SIGUSR1 print nopass nostop
tb loop
r
disass
b if condA ()
commands
echo A\n
end
stepi
b if condB ()
commands
echo B\n
end
#set debug infrun 1
stepi
stepi
 <-- CTRL-C it here, it keeps scrolling signal deliveries

gcc -o 1 1.c -O2 -Wall -g -pthread
valgrind --db-attach=yes --num-callers=50 ./gdb -nx -x ./1.cmd
x86_64-fedora13-linux-gnu


No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.

A comment is welcome but it seems safe to me.


Thanks,
Jan


gdb/
2010-11-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* infrun.c (struct inferior_thread_state) <step_resume_breakpoint>:
	New field.
	(save_inferior_thread_state): Save STEP_RESUME_BREAKPOINT.
	(restore_inferior_thread_state): Restore STEP_RESUME_BREAKPOINT.

--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6044,6 +6044,7 @@ struct inferior_thread_state
   enum target_signal stop_signal;
   CORE_ADDR stop_pc;
   struct regcache *registers;
+  struct breakpoint *step_resume_breakpoint;
 
   /* Format of SIGINFO or NULL if it is not present.  */
   struct gdbarch *siginfo_gdbarch;
@@ -6096,6 +6097,9 @@ save_inferior_thread_state (void)
 
   inf_state->registers = regcache_dup (regcache);
 
+  inf_state->step_resume_breakpoint = tp->step_resume_breakpoint;
+  tp->step_resume_breakpoint = NULL;
+
   return inf_state;
 }
 
@@ -6127,6 +6131,11 @@ restore_inferior_thread_state (struct inferior_thread_state *inf_state)
     /* NB: The register write goes through to the target.  */
     regcache_cpy (regcache, inf_state->registers);
   regcache_xfree (inf_state->registers);
+
+  /* Check first for stale STEP_RESUME_BREAKPOINT after an infcall.  */
+  gdb_assert (tp->step_resume_breakpoint == NULL);
+  tp->step_resume_breakpoint = inf_state->step_resume_breakpoint;
+
   xfree (inf_state->siginfo_data);
   xfree (inf_state);
 }

Re: [patch] Fix stale tp->step_resume_breakpoint

[Pedro Alves]

On Tuesday 02 November 2010 00:43:01, Jan Kratochvil wrote:
> A comment is welcome but it seems safe to me.

I think this raises an obvious question, and hints at
a larger issue: if you find you you need to tuck away step_resume_breakpoint,
then, how come you don't need to do the same for all the other execution
command state?  (step_range_start, step_range_end, step_frame_id,
continuations, etc.).
I'd assume that in the use case you trip on step_resume_breakpoint
troubles, you'd also be losing thread stepping state (or state
for any other execution command), thus your thread would end up
running free, forgetting about the previous command that was
going on before the infcall.  Is that not the case?

-- 
Pedro Alves

Re: [patch] Fix stale tp->step_resume_breakpoint

[Jan Kratochvil]

On Tue, 02 Nov 2010 02:05:18 +0100, Pedro Alves wrote:
> On Tuesday 02 November 2010 00:43:01, Jan Kratochvil wrote:
> > A comment is welcome but it seems safe to me.
> 
> I think this raises an obvious question, and hints at
> a larger issue: if you find you you need to tuck away step_resume_breakpoint,
> then, how come you don't need to do the same for all the other execution
> command state?  (step_range_start, step_range_end, step_frame_id,
> continuations, etc.).
> I'd assume that in the use case you trip on step_resume_breakpoint
> troubles, you'd also be losing thread stepping state (or state
> for any other execution command), thus your thread would end up
> running free, forgetting about the previous command that was
> going on before the infcall.  Is that not the case?

Currently I do not have a meaningful reproducer for it.

But I see step_resume_breakpoint on its own does not make much sense without
the associated information so I will try to save more info.


Thanks,
Jan

obsolete: Re: [patch] Fix stale tp->step_resume_breakpoint

[Jan Kratochvil]

thread obsoleted by:
	[patch 3/3] Fix stale tp->step_resume_breakpoint
	http://sourceware.org/ml/gdb-patches/2010-11/msg00386.html