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]

GDB crash while stepping into function.


We noticed the following crash on sparc-solaris and x86-solaris:
 
    (gdb) start
    [...]
    Temporary breakpoint 1, mainpack () at mainpack.adb:4
    4         pack1.proc1("mainpack");  --  START
    (gdb) step
    zsh: 25763 bus error (core dumped)
    /nova.a/brobecke/bld/gdb-public/gdb/gdb mainpack
    
This is a case of a dangling pointer. Looking at handle_inferior_event,
we see that frame gets recomputed as follow:
 
        /* Re-fetch current thread's frame in case we did a
           "goto process_event_stop_test" above.  */
        frame = get_current_frame ();
 
But shortly after, we call delete_step_resume_breakpoint:

        case BPSTAT_WHAT_STEP_RESUME:
          [...]
          delete_step_resume_breakpoint (ecs->event_thread);
    
This eventually causes remove_breakpoint to be called, which itself
calls switch_to_program_space_and_thread, which naturally causes
a thread_switch, thus leading to the frame-cache being reinitialized:

        void
        switch_to_thread (ptid_t ptid)
        { 
          [...]
          reinit_frame_cache ();

And as a result, our "frame" variable becomes a dangling pointer.
The obvious bandaid is to refectch the current_frame after the breakpoint
handling is complete.  This is what I have done for now.

However, I think we should consider ditching the variable and replacing
it everywhere with a call to get_current_frame().  handle_inferior_event
is such a large and complex function that it is extremely easy for
a change to cause the same sort of problem; too easy IMO for the usual
difficulties faced when tracking down the source of the memory corruption.
My suggestion is to add the following define at the beginning of
the handle_inferior_event function:

    /* We purposefully never store the current frame in a variable inside
       this function, because the frame cache often gets invalidated
       as a side-effect of the processing being done here.  Given the size
       and complexity of this function, there is too much risk that this
       variable becomes a dangling pointer.  And thus, we use this macro
       to make sure that the correct current frame is always accessed,
       while making it clear that this is intended and should not be changed
       (without the macro, the developer might be tempted to optimize
       the code by re-introducing a variable).  */
    #define hie_frame get_current_frame ()

Tested on sparc-solaris.

gdb/ChangeLog:

        GDB crash while stepping into function.
        * infrun.c (handle_inferior_event): Refetch the current frame
        after handling what.main_action, in case that pointer became
        dangling.

-- 
Joel
gdb/ChangeLog:

        GDB crash while stepping into function.
        * infrun.c (handle_inferior_event): Refetch the current frame
        after handling what.main_action, in case that pointer became
        dangling.
---
 gdb/infrun.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index a657026..a6b5dd6 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4065,6 +4065,11 @@ infrun: not switching back to stepped thread, it has vanished\n");
       return;
     }
 
+  /* Re-fetch current thread's frame in case the code above caused
+     the frame cache to be re-initialized, making our FRAME variable
+     a dangling pointer.  */
+  frame = get_current_frame ();
+
   /* If stepping through a line, keep going if still within it.
 
      Note that step_range_end is the address of the first instruction
-- 
1.5.4.3


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