[RFC] breakpoint.c: Fix nasty problem with msvcrt DLL on Windows

[RFC] breakpoint.c: Fix nasty problem with msvcrt DLL on Windows

[Pierre Muller]

  I ran testsuite on Cygwin and found out that
all pascal tests were failing. Further inverstigation showed
that the same problem happens more than 300 times,
so that it is not pascal specific.

  It seems to be related to the fact that the Google toolbar 
is installed on that machine: 

  On startup of executables, a Google specific DLL gets loaded,
which itself load msvcrt.dll, a DLL that contains a 'longjmp' function.
This 'longjmp' functions triggers the creation of a bp_longjmp_master
breakpoint type.
  But the Google toolbar also unloads itself during initialization
and this triggers unloading of msvcrt.dll.
  Here a GDB bug appears: the unloading of msvcrt DLL
does not trigger the removal of the corresponding longjmp breakpoint,
which then leads to the error when trying to step
that executable.
  I wrote a little test C code that allows to check
for the bug on any Windows system with msvcrt.dll installed.
The bug is probably not windows specific, and would probably 
show on each dynamically loaded library that contains a 'longjmp' function
and gets unloaded.

  Does anyone know if this is a known problem?
I was able to 'fix' it by setting  loc->shlib_disabled to 1
for the longjmp_master breakpoint.
  But maybe we should completely remove the bp_longjmp_master
breakpoint as it is an internal breakpoint, but I
am not familiar enough with all the subtleties of breakpoint code
to be sure that this is not dangerous.
  I am also not sure about the code in set_longjmp_breakpoint 
is it safe to assume that b->loc is always non NULL
for a bp_longjmp_master, or should that be modified?


  Comments most welcome,

Pierre Muller


>>>>>>>>>>>>>>>>>>>>Start of transcript
(gdb) start
Temporary breakpoint 1 at 0x401056: file
../../../src/gdb/testsuite/gdb.pascal/f
loats.pas, line 25.
Starting program:
/usr/local/src/gdbcvs/build-norm/gdb/testsuite/gdb.pascal/floa
ts.exe
gdb: windows_init_thread_list
gdb: kernel event for pid=1544 tid=d7c code=CREATE_PROCESS_DEBUG_EVENT)
[New Thread 1544.0xd7c]
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/ntdll.dll" at 0x7c910000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/kernel32.dll" at 0x7c800000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/cygwin/bin/cygwin1.dll" at 0x61000000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/ADVAPI32.DLL" at 0x77da0000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/RPCRT4.dll" at 0x77e50000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/Secur32.dll" at 0x77fc0000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=EXCEPTION_DEBUG_EVENT)
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=350 code=CREATE_THREAD_DEBUG_EVENT)
[New Thread 1544.0x350]
ContinueDebugEvent (cpid=1544, ctid=350, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=OUTPUT_DEBUG_STRING_EVENT)
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=OUTPUT_DEBUG_STRING_EVENT)
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/user32.dll" at 0x7e390000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/GDI32.dll" at 0x77ef0000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/IMM32.DLL" at 0x76320000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/PROGRA~1/Google/GOOGLE~2/GOEC62~1.DLL" at
0x480000
00.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/WS2_32.dll" at 0x719f0000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/msvcrt.dll" at 0x77be0000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=LOAD_DLL_DEBUG_EVENT)
gdb: Loading dll "/cygdrive/c/WINDOWS/system32/WS2HELP.dll" at 0x719e0000.
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=UNLOAD_DLL_DEBUG_EVENT)
gdb: Unloading dll "/cygdrive/c/PROGRA~1/Google/GOOGLE~2/GOEC62~1.DLL".
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=UNLOAD_DLL_DEBUG_EVENT)
gdb: Unloading dll "/cygdrive/c/WINDOWS/system32/WS2_32.dll".
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=UNLOAD_DLL_DEBUG_EVENT)
gdb: Unloading dll "/cygdrive/c/WINDOWS/system32/WS2HELP.dll".
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=UNLOAD_DLL_DEBUG_EVENT)
gdb: Unloading dll "/cygdrive/c/WINDOWS/system32/msvcrt.dll".
ContinueDebugEvent (cpid=1544, ctid=d7c, DBG_CONTINUE);
gdb: kernel event for pid=1544 tid=d7c code=EXCEPTION_DEBUG_EVENT)

Temporary breakpoint 1, _p__M0_main_program ()
    at ../../../src/gdb/testsuite/gdb.pascal/floats.pas:25
25      begin
(gdb) maint inf b
Num     Type           Disp Enb Address    What
-10     longjmp master keep n   0x61093868 <longjmp> inf 1
-11     longjmp master keep n   0x77c06d74  inf 1

(gdb) n
Warning:
Cannot insert breakpoint -11.
Error accessing memory address 0x77c06d74: Input/Output error.

(gdb)

>>>>>>>>>>>>>>>>>>>>End of transcript

This -11 longjmp breakpoint should have been removed when msvcrtl.dll was
unloaded, but this does not seem to work correctly.

ChangeLog entry:
2010-08-05  Pierre Muller  <muller@ics.u-strasbg.fr>

        * breakpoint.c (set_longjmp_breakpoint): Only insert breakpoints
        if the dynamic library was not unloaded.
        (disable_breakpoints_in_unloaded_shlib): Disable also 'longjmp'
        breakpoints.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.506
diff -u -p -r1.506 breakpoint.c
--- breakpoint.c        3 Aug 2010 22:35:41 -0000       1.506
+++ breakpoint.c        5 Aug 2010 10:03:14 -0000
@@ -5573,10 +5573,12 @@ set_longjmp_breakpoint (int thread)
   /* To avoid having to rescan all objfile symbols at every step,
      we maintain a list of continually-inserted but always disabled
      longjmp "master" breakpoints.  Here, we simply create momentary
-     clones of those and enable them for the requested thread.  */
+     clones of those and enable them for the requested thread.
+     Do not try to insert 'longjmp' breakpoints of unloaded libraries.  */
   ALL_BREAKPOINTS_SAFE (b, temp)
     if (b->pspace == current_program_space
-       && b->type == bp_longjmp_master)
+       && b->type == bp_longjmp_master
+        && b->loc && !b->loc->shlib_disabled)
       {
        struct breakpoint *clone = clone_momentary_breakpoint (b);

@@ -5793,6 +5795,8 @@ disable_breakpoints_in_unloaded_shlib (s
        && !loc->shlib_disabled
        && (b->type == bp_breakpoint
            || b->type == bp_jit_event
+           || b->type == bp_longjmp
+           || b->type == bp_longjmp_master
            || b->type == bp_hardware_breakpoint)
        && solib_contains_address_p (solib, loc->address))
       {

PS: Here is a simple test code to reproduce the problem:
1> #include <windows.h>
2> 
3> int
4> main ()
5> 
6> {
7>   HMODULE h = LoadLibraryA ("msvcrt.dll");
8>   void * func = GetProcAddress (h, "longjmp");
9>   FreeLibrary (h);
10>  printf ("Address of longjmp is 0x%x\n", (long) func);
11> }

  You can compile this on Windows and use 
(gdb) start
(gdb) set debugevents on
(gdb) next
until you reach line 10,
from there 'next' will fail with the 'Cannot insert breakpoint' error
on current head GDB, 
'cont' is still working, because in that case the longjmp internal
breakpoints
are not inserted. 
  With my patch you will just get a warning about 'Temporarily disabling
breakpoints for unloaded
share library ...msvcrt.dll". This warning should probably be removed
also for an internal breakpoint.

RE: [RFC] breakpoint.c: Fix nasty problem with msvcrt DLL on Windows

[Pierre Muller]

  Just one short follow-up:
I ran the testsuite with my patch on a x86_64 machine
and got two regressions:

First:
< PASS: gdb.base/attach.exp: attach1 detach
---
> FAIL: gdb.base/attach.exp: attach1 detach
Detaching from program:
/home/muller/auto-test-gdb/build/gdb/testsuite/gdb.base/
attach, process 16517^M
warning: Temporarily disabling breakpoints for unloaded shared library
"/lib/lib
c.so.6"^M
(gdb) FAIL: gdb.base/attach.exp: attach1 detach

The failure comes from the new warning introduced by my patch.

Second:
< PASS: gdb.server/ext-attach.exp: detach
---
> FAIL: gdb.server/ext-attach.exp: detach
Same reason.

With the updated patch below these two regressions disappear.

Pierre


2010-08-05  Pierre Muller  <muller@ics.u-strasbg.fr>

	* breakpoint.c (set_longjmp_breakpoint): Only insert breakpoints
	if the dynamic library was not unloaded.
	(disable_breakpoints_in_unloaded_shlib): Disable also 'longjmp'
	breakpoints.

Index: src/gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.506
diff -u -p -r1.506 breakpoint.c
--- src/gdb/breakpoint.c	3 Aug 2010 22:35:41 -0000	1.506
+++ src/gdb/breakpoint.c	5 Aug 2010 10:03:14 -0000
@@ -5573,10 +5573,12 @@ set_longjmp_breakpoint (int thread)
   /* To avoid having to rescan all objfile symbols at every step,
      we maintain a list of continually-inserted but always disabled
      longjmp "master" breakpoints.  Here, we simply create momentary
-     clones of those and enable them for the requested thread.  */
+     clones of those and enable them for the requested thread.
+     Do not try to insert 'longjmp' breakpoints of unloaded libraries.  */
   ALL_BREAKPOINTS_SAFE (b, temp)
     if (b->pspace == current_program_space
-	&& b->type == bp_longjmp_master)
+	&& b->type == bp_longjmp_master
+        && b->loc && !b->loc->shlib_disabled)
       {
 	struct breakpoint *clone = clone_momentary_breakpoint (b);
 
@@ -5801,7 +5805,8 @@ disable_breakpoints_in_unloaded_shlib (s
 	   succeeding so we must mark the breakpoint as not inserted
 	   to prevent future errors occurring in remove_breakpoints.  */
 	loc->inserted = 0;
-	if (!disabled_shlib_breaks)
+	if (!disabled_shlib_breaks && b->type != bp_longjmp_master
+	    && b->type != bp_longjmp)
 	  {
 	    target_terminal_ours_for_output ();
 	    warning (_("Temporarily disabling breakpoints for unloaded
shared library \"%s\""),

Re: [RFC] breakpoint.c: Fix nasty problem with msvcrt DLL on Windows

[Pedro Alves]

On Thursday 05 August 2010 12:35:51, Pierre Muller wrote:
> (gdb) maint inf b
> Num     Type           Disp Enb Address    What
> -10     longjmp master keep n   0x61093868 <longjmp> inf 1
> -11     longjmp master keep n   0x77c06d74  inf 1
> 
> (gdb) n
> Warning:
> Cannot insert breakpoint -11.
> Error accessing memory address 0x77c06d74: Input/Output error.

1. Did we really try to insert a Enb=n breakpoint?
2. Did we really try to insert a longjmp master breakpoint?

If yes to any of those, something else is broken.

-- 
Pedro Alves

RE: [RFC] breakpoint.c: Fix nasty problem with msvcrt DLL on Windows

[Pierre Muller]

 If I understood the code correctly,
"longjmp master" breakpoints type are internal breakpoints
that stay always disabled and never get enabled.

  Their function is to list all possible functions
that might do long jumps (i.e. modify the stack directly)
and trigger the creation of a normal "longjmp" breakpoint,
via a call to "set_longjmp_breakpoint" function.
This function is called only once in step_1 function in infcmd.c 
source.

The normal "longjmp" breakpoints are deleted at each stop,
by a call to "delete_longjmp_breakpoint" (2 such calls
exist, one is part of a cleanup in case an error
appears).

  Thus, when calling "maint info breakpoints" command,
only "longjmp master" internal breakpoint types are listed.

  If my analysis is correct, the answer to your two questions
is no.
 
Pierre

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : Thursday, August 12, 2010 5:43 PM
> À : gdb-patches@sourceware.org
> Cc : Pierre Muller
> Objet : Re: [RFC] breakpoint.c: Fix nasty problem with msvcrt DLL on
> Windows
> 
> On Thursday 05 August 2010 12:35:51, Pierre Muller wrote:
> > (gdb) maint inf b
> > Num     Type           Disp Enb Address    What
> > -10     longjmp master keep n   0x61093868 <longjmp> inf 1
> > -11     longjmp master keep n   0x77c06d74  inf 1
> >
> > (gdb) n
> > Warning:
> > Cannot insert breakpoint -11.
> > Error accessing memory address 0x77c06d74: Input/Output error.
> 
> 1. Did we really try to insert a Enb=n breakpoint?
> 2. Did we really try to insert a longjmp master breakpoint?
> 
> If yes to any of those, something else is broken.
> 
> --
> Pedro Alves

Re: [RFC] breakpoint.c: Fix nasty problem with msvcrt DLL on Windows

[Pedro Alves]

(Please don't top post.)

On Friday 13 August 2010 09:46:46, Pierre Muller wrote:
> > On Thursday 05 August 2010 12:35:51, Pierre Muller wrote:
> > > (gdb) maint inf b
> > > Num     Type           Disp Enb Address    What
> > > -10     longjmp master keep n   0x61093868 <longjmp> inf 1
> > > -11     longjmp master keep n   0x77c06d74  inf 1
> > >
> > > (gdb) n
> > > Warning:
> > > Cannot insert breakpoint -11.
> > > Error accessing memory address 0x77c06d74: Input/Output error.
> > 
> > 1. Did we really try to insert a Enb=n breakpoint?
> > 2. Did we really try to insert a longjmp master breakpoint?
> > 
> > If yes to any of those, something else is broken.

>  If I understood the code correctly,
> "longjmp master" breakpoints type are internal breakpoints
> that stay always disabled and never get enabled.

(...)

>   If my analysis is correct, the answer to your two questions
> is no.

Your analysis is correct.  But then why did gdb print
"Cannot insert breakpoint -11.", with -11 being the
number of one of the internal longjmp master breakpoints?
Are we copying the breakpoint number when creating the
longjmp momentary breakpoints from the master, meaning that's
a red herring (though one that I'd like fixed), or is there
something else really broken?

-- 
Pedro Alves

RE: [RFC] breakpoint.c: Fix nasty problem with msvcrt DLL on Windows

[Pierre Muller]

> > > -11     longjmp master keep n   0x77c06d74  inf 1
> > >
> > > (gdb) n
> > > Warning:
> > > Cannot insert breakpoint -11.
> > > Error accessing memory address 0x77c06d74: Input/Output error.

> Your analysis is correct.  But then why did gdb print
> "Cannot insert breakpoint -11.", with -11 being the
> number of one of the internal longjmp master breakpoints?
> Are we copying the breakpoint number when creating the
> longjmp momentary breakpoints from the master, meaning that's
> a red herring (though one that I'd like fixed), or is there
> something else really broken?


  I was unable to reproduce that issue,
and I suspect that this is just an error on me side:
I probably just copied parts of two different gdb sessions.

  When trying to reproduce it, I always got internal (negative) numbers
that were different (more negative, meaning created later)
for the "longjmp" breakpoints as compared to their "longjmp master"
equivalent.

  Sorry to have generated that confusion,

Pierre

  PS: could you test and confirm the bug report 11886?

[PING][RFC] breakpoint.c: Fix nasty problem with msvcrt DLL on Windows

[Pierre Muller]

  We still didn't take any decision on this patch.
The problem is reported in
http://sourceware.org/bugzilla/show_bug.cgi?id=11886

  I cannot debug anything on one of my Windows PC
because it loads and unloads msvcrt.dll at startup;
which leads to the problem.


Pierre Muller
Pascal language support maintainer for GDB




> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pierre Muller
> Envoyé : Friday, August 13, 2010 3:50 PM
> À : 'Pedro Alves'; gdb-patches@sourceware.org
> Objet : RE: [RFC] breakpoint.c: Fix nasty problem with msvcrt DLL on
> Windows
> 
> > > > -11     longjmp master keep n   0x77c06d74  inf 1
> > > >
> > > > (gdb) n
> > > > Warning:
> > > > Cannot insert breakpoint -11.
> > > > Error accessing memory address 0x77c06d74: Input/Output error.
> 
> > Your analysis is correct.  But then why did gdb print
> > "Cannot insert breakpoint -11.", with -11 being the
> > number of one of the internal longjmp master breakpoints?
> > Are we copying the breakpoint number when creating the
> > longjmp momentary breakpoints from the master, meaning that's
> > a red herring (though one that I'd like fixed), or is there
> > something else really broken?
> 
> 
>   I was unable to reproduce that issue,
> and I suspect that this is just an error on me side:
> I probably just copied parts of two different gdb sessions.
> 
>   When trying to reproduce it, I always got internal (negative) numbers
> that were different (more negative, meaning created later)
> for the "longjmp" breakpoints as compared to their "longjmp master"
> equivalent.
> 
>   Sorry to have generated that confusion,
> 
> Pierre
> 
>   PS: could you test and confirm the bug report 11886?