[RFC] fix win32-nat failure

[RFC] fix win32-nat failure

[Pierre Muller]

  Current GDB head has a problem with win32-nat:

When starting ./gdb ./gdb
(gdb) run
Starting program: /usr/local/src/gdbcvs/build-bare/gdb/gdb.exe ./gdb
../../purecvs/gdb/inferior.c:41: internal-error: current_inferior: Assertion
`inf' failed.



This comes from a call to
Breakpoint 1, internal_error (file=0x6525d0 "../../purecvs/gdb/inferior.c",
    line=41, string=0x6525b1 "%s: Assertion `%s' failed.")
    at ../../purecvs/gdb/utils.c:897
897       va_start (ap, string);
(top-gdb) bt
#0  internal_error (file=0x6525d0 "../../purecvs/gdb/inferior.c", line=41,
    string=0x6525b1 "%s: Assertion `%s' failed.")
    at ../../purecvs/gdb/utils.c:897
#1  0x0043d116 in current_inferior () at ../../purecvs/gdb/inferior.c:41
#2  0x0042e1bf in terminal_inferior () at ../../purecvs/gdb/inflow.c:280
During symbol reading, struct/union type gets multiply defined: struct
language_
defn.
#3  0x004753bc in do_initial_win32_stuff (pid=4996, attaching=0)

I was able to fix this by setting inferior_ptid before terminal_inferior was
called,
but I did this using also the ThreadId available when starting with
CreateProcess.
  But there is no thread number available for processes to which we attach,
thus I simply passed zero in that case...

  I don't know, maybe the correct fix is to only set inferior_ptid
to ptid_build(pid, 0, 0)

  Anyhow, it would be nice if this could be fixed ASAP.


Pierre Muller
Pascal language support maintainer for GDB






Index: gdb/win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.164
diff -u -p -r1.164 win32-nat.c
--- gdb/win32-nat.c     22 Sep 2008 15:21:30 -0000      1.164
+++ gdb/win32-nat.c     1 Oct 2008 12:10:33 -0000
@@ -1521,7 +1521,7 @@ win32_wait (ptid_t ptid, struct target_w
 }

 static void
-do_initial_win32_stuff (DWORD pid, int attaching)
+do_initial_win32_stuff (DWORD pid, DWORD tid, int attaching)
 {
   extern int stop_after_trap;
   int i;
@@ -1548,6 +1548,7 @@ do_initial_win32_stuff (DWORD pid, int a
   init_wait_for_inferior ();

   inf = add_inferior (pid);
+  inferior_ptid = ptid_build (pid, 0, tid);
   inf->attach_flag = attaching;

   terminal_init_inferior_with_pgrp (pid);
@@ -1729,7 +1730,7 @@ win32_attach (char *args, int from_tty)
       gdb_flush (gdb_stdout);
     }

-  do_initial_win32_stuff (pid, 1);
+  do_initial_win32_stuff (pid, 0, 1);
   target_terminal_ours ();
 }

@@ -1939,7 +1940,7 @@ win32_create_inferior (char *exec_file,
   else
     saw_create = 0;

-  do_initial_win32_stuff (pi.dwProcessId, 0);
+  do_initial_win32_stuff (pi.dwProcessId, pi.dwThreadId, 0);

   /* win32_continue (DBG_CONTINUE, -1); */
 }

Re: [RFC] fix win32-nat failure

[Pedro Alves]

Huh, weird, I did test this on Cygwin, but looks like I
missed something.  :-(

Thanks, I'll take a look.

On Wednesday 01 October 2008 13:25:08, Pierre Muller wrote:
> 
>   Current GDB head has a problem with win32-nat:
> 
> When starting ./gdb ./gdb
> (gdb) run
> Starting program: /usr/local/src/gdbcvs/build-bare/gdb/gdb.exe ./gdb
> ../../purecvs/gdb/inferior.c:41: internal-error: current_inferior: Assertion
> `inf' failed.
> 
> 
> 
> This comes from a call to
> Breakpoint 1, internal_error (file=0x6525d0 "../../purecvs/gdb/inferior.c",
>     line=41, string=0x6525b1 "%s: Assertion `%s' failed.")
>     at ../../purecvs/gdb/utils.c:897
> 897       va_start (ap, string);
> (top-gdb) bt
> #0  internal_error (file=0x6525d0 "../../purecvs/gdb/inferior.c", line=41,
>     string=0x6525b1 "%s: Assertion `%s' failed.")
>     at ../../purecvs/gdb/utils.c:897
> #1  0x0043d116 in current_inferior () at ../../purecvs/gdb/inferior.c:41
> #2  0x0042e1bf in terminal_inferior () at ../../purecvs/gdb/inflow.c:280
> During symbol reading, struct/union type gets multiply defined: struct
> language_
> defn.
> #3  0x004753bc in do_initial_win32_stuff (pid=4996, attaching=0)
> 
> I was able to fix this by setting inferior_ptid before terminal_inferior was
> called,
> but I did this using also the ThreadId available when starting with
> CreateProcess.
>   But there is no thread number available for processes to which we attach,
> thus I simply passed zero in that case...
> 
>   I don't know, maybe the correct fix is to only set inferior_ptid
> to ptid_build(pid, 0, 0)
> 
>   Anyhow, it would be nice if this could be fixed ASAP.
> 
> 
> Pierre Muller
> Pascal language support maintainer for GDB
> 
> 
> 
> 
> 
> 
> Index: gdb/win32-nat.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/win32-nat.c,v
> retrieving revision 1.164
> diff -u -p -r1.164 win32-nat.c
> --- gdb/win32-nat.c     22 Sep 2008 15:21:30 -0000      1.164
> +++ gdb/win32-nat.c     1 Oct 2008 12:10:33 -0000
> @@ -1521,7 +1521,7 @@ win32_wait (ptid_t ptid, struct target_w
>  }
> 
>  static void
> -do_initial_win32_stuff (DWORD pid, int attaching)
> +do_initial_win32_stuff (DWORD pid, DWORD tid, int attaching)
>  {
>    extern int stop_after_trap;
>    int i;
> @@ -1548,6 +1548,7 @@ do_initial_win32_stuff (DWORD pid, int a
>    init_wait_for_inferior ();
> 
>    inf = add_inferior (pid);
> +  inferior_ptid = ptid_build (pid, 0, tid);
>    inf->attach_flag = attaching;
> 
>    terminal_init_inferior_with_pgrp (pid);
> @@ -1729,7 +1730,7 @@ win32_attach (char *args, int from_tty)
>        gdb_flush (gdb_stdout);
>      }
> 
> -  do_initial_win32_stuff (pid, 1);
> +  do_initial_win32_stuff (pid, 0, 1);
>    target_terminal_ours ();
>  }
> 
> @@ -1939,7 +1940,7 @@ win32_create_inferior (char *exec_file,
>    else
>      saw_create = 0;
> 
> -  do_initial_win32_stuff (pi.dwProcessId, 0);
> +  do_initial_win32_stuff (pi.dwProcessId, pi.dwThreadId, 0);
> 
>    /* win32_continue (DBG_CONTINUE, -1); */
>  }
> 
> 



-- 
Pedro Alves

Re: [RFC] fix win32-nat failure

[Pedro Alves]

On Wednesday 01 October 2008 13:25:08, Pierre Muller wrote:

>   Current GDB head has a problem with win32-nat:
> 
> When starting ./gdb ./gdb
> (gdb) run
> Starting program: /usr/local/src/gdbcvs/build-bare/gdb/gdb.exe ./gdb
> ../../purecvs/gdb/inferior.c:41: internal-error: current_inferior: Assertion
> `inf' failed.

Ah, I know why I didn't see this happening.  When running GDB under
GDB itself, GDB considers that it doesn't have a terminal at all, so
this code path isn't exercised.  Maybe this applies to the testsuite
as well.  :-/

> I was able to fix this by setting inferior_ptid before terminal_inferior was
> called,
> but I did this using also the ThreadId available when starting with
> CreateProcess.
>   But there is no thread number available for processes to which we attach,
> thus I simply passed zero in that case...

>   I don't know, maybe the correct fix is to only set inferior_ptid
> to ptid_build(pid, 0, 0)

Yes, you don't need the tid at this point even when creating instead
of attaching.  A smaller change would be something like this:

+  inferior_ptid = pid_to_ptid (pid);

   terminal_init_inferior_with_pgrp (pid);

Perhaps also adding a comment saying something like:

  /* Make the new process the current inferior, so terminal handling
     can rely on it.  When attaching, we don't know about any thread
     id here, but that's OK --- nothing should be referencing the
     current thread until we report an event out of win32_wait.  */

Do you want to give it a try and confirm on your end?

-- 
Pedro Alves

RE: [RFC] fix win32-nat failure

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : Wednesday, October 01, 2008 8:09 PM
> À : gdb-patches@sourceware.org
> Cc : Pierre Muller
> Objet : Re: [RFC] fix win32-nat failure
> 
> On Wednesday 01 October 2008 13:25:08, Pierre Muller wrote:
> 
> >   Current GDB head has a problem with win32-nat:
> >
> > When starting ./gdb ./gdb
> > (gdb) run
> > Starting program: /usr/local/src/gdbcvs/build-bare/gdb/gdb.exe ./gdb
> > ../../purecvs/gdb/inferior.c:41: internal-error: current_inferior:
> Assertion
> > `inf' failed.
> 
> Ah, I know why I didn't see this happening.  When running GDB under
> GDB itself, GDB considers that it doesn't have a terminal at all, so
> this code path isn't exercised.  Maybe this applies to the testsuite
> as well.  :-/

  Well, in fact, I don't understand this statement,
after reading it, I suspected that this had to do with the fact that
I use a lot the 'set new-console on' option,
but even without this I still get the same assertion failed message,
so I don't understand why you did not also get it.
 
> > I was able to fix this by setting inferior_ptid before
> terminal_inferior was
> > called,
> > but I did this using also the ThreadId available when starting with
> > CreateProcess.
> >   But there is no thread number available for processes to which we
> attach,
> > thus I simply passed zero in that case...
> 
> >   I don't know, maybe the correct fix is to only set inferior_ptid
> > to ptid_build(pid, 0, 0)
> 
> Yes, you don't need the tid at this point even when creating instead
> of attaching.  A smaller change would be something like this:
> 
> +  inferior_ptid = pid_to_ptid (pid);
> 
>    terminal_init_inferior_with_pgrp (pid);
> 
> Perhaps also adding a comment saying something like:
> 
>   /* Make the new process the current inferior, so terminal handling
>      can rely on it.  When attaching, we don't know about any thread
>      id here, but that's OK --- nothing should be referencing the
>      current thread until we report an event out of win32_wait.  */
> 
> Do you want to give it a try and confirm on your end?

  OK, let me try this simpler patch out,
I will send a RFA if that works OK for me.


Pierre Muller
Pascal language support maintainer for GDB

[RFA] fix win32-nat failure

[Pierre Muller]

I ran a testsuite on cygwin with the patch below,
I got this results:

                === gdb Summary ===

# of expected passes            11190
# of unexpected failures        469
# of expected failures          58
# of unknown successes          2
# of known failures             110
# of unresolved testcases       40
# of untested testcases         15
# of unsupported tests          23
/usr/local/src/gdbcvs/build-bare/gdb/testsuite/../../gdb/gdb version
6.8.50.200
81002-cvs -nx

make: *** [check] Error 1

This seems like a "normal" outcome for cygwin native gdb.

Thus I would like to get approval for the following patch,


Pierre Muller
Pascal language support maintainer for GDB




ChangeLog entry:

2008-10-02  Pierre Muller  <muller@ics.u-strasbg.fr>
		Pedro Alves  <pedro@codesourcery.com>

	* win32-nat.c (do_initial_win32_stuff): Set inferior_ptid.


Index: gdb/win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.164
diff -u -p -r1.164 win32-nat.c
--- gdb/win32-nat.c     22 Sep 2008 15:21:30 -0000      1.164
+++ gdb/win32-nat.c     2 Oct 2008 08:59:25 -0000
@@ -1550,6 +1550,12 @@ do_initial_win32_stuff (DWORD pid, int a
   inf = add_inferior (pid);
   inf->attach_flag = attaching;

+  /* Make the new process the current inferior, so terminal handling
+     can rely on it.  When attaching, we don't know about any thread
+     id here, but that's OK --- nothing should be referencing the
+     current thread until we report an event out of win32_wait.  */
+  inferior_ptid = pid_to_ptid (pid);
+
   terminal_init_inferior_with_pgrp (pid);
   target_terminal_inferior ();

Re: [RFC] fix win32-nat failure

[Pedro Alves]

On Thursday 02 October 2008 09:39:32, Pierre Muller wrote:

>   Well, in fact, I don't understand this statement,
> after reading it, I suspected that this had to do with the fact that
> I use a lot the 'set new-console on' option,
> but even without this I still get the same assertion failed message,
> so I don't understand why you did not also get it.

Sorry for being vague.  I just meant that this crashes:

 $ ./gdb --quiet ./test.exe
 (gdb) start
 Temporary breakpoint 1 at 0x40107a: file test.c, line 3.
 Starting program: /home/pedro/gdb/baseline/build/gdb/test.exe
 ../../src/gdb/inferior.c:41: internal-error: current_inferior: Assertion `inf' failed.
 A problem internal to GDB has been detected,
 further debugging may prove unreliable.
 Quit this debugging session? (y or n)

But this doesn't:

 $ gdb --args ./gdb ./test.exe
 GNU gdb 6.8.0.20080328-cvs (cygwin-special)
 (top-gdb) run
 Starting program: /home/pedro/gdb/baseline/build/gdb/gdb.exe ./test.exe
...
 (gdb) start
 Temporary breakpoint 1 at 0x40107a: file test.c, line 3.
 Starting program: /home/pedro/gdb/baseline/build/gdb/test.exe
 [New Thread 2196.0x2e0]
 [New Thread 2196.0xbe8]
 
 Temporary breakpoint 1, main () at test.c:3
 3         return 0;
 (gdb)

Because:

 (gdb) i
 (top-gdb) p gdb_has_a_terminal()
 $1 = 0

hence, the crash site is skipped (inflow.c):

 178     void
 179     terminal_init_inferior_with_pgrp (int pgrp)
 180     {
 181       if (gdb_has_a_terminal ())
 182         {

 234     void
 235     terminal_inferior (void)
 236     {
 237       if (gdb_has_a_terminal () && terminal_is_ours
 238           && inferior_ttystate != NULL
 239           && inferior_thisrun_terminal == 0)
 240         {

> OK, let me try this simpler patch out,
> I will send a RFA if that works OK for me.

Thanks!

-- 
Pedro Alves

Re: [RFA] fix win32-nat failure

[Pedro Alves]

On Thursday 02 October 2008 12:53:44, Pierre Muller wrote:
> I ran a testsuite on cygwin with the patch below,
> I got this results:
> 
>                 === gdb Summary ===
> 
> # of expected passes            11190
> # of unexpected failures        469
> # of expected failures          58
> # of unknown successes          2
> # of known failures             110
> # of unresolved testcases       40
> # of untested testcases         15
> # of unsupported tests          23
> /usr/local/src/gdbcvs/build-bare/gdb/testsuite/../../gdb/gdb version
> 6.8.50.200
> 81002-cvs -nx
> 
> This seems like a "normal" outcome for cygwin native gdb.

Hmmm, I don't have a good baseline stored to compare this to, but, from
memory, this looks worse than is was a few weeks ago...  The failure count
should be much lower currently, due to both the fix that forced unbuffered
stdout on Cygwin, and the skip __main fix.

Oh, I'm running the testsuite too, and I'm getting several crashes
like these, which I had never seen before:

 $ ./gdb /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.cp/class2
 GNU gdb (GDB) 6.8.50.20081002-cvs
 Copyright (C) 2008 Free Software Foundation, Inc.
 License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
 This is free software: you are free to change and redistribute it.
 There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
 and "show warranty" for details.
 This GDB was configured as "i686-pc-cygwin".
 For bug reporting instructions, please see:
 <http://www.gnu.org/software/gdb/bugs/>...
 BFD: BFD (GNU Binutils) 2.19.50.20081002 internal error, aborting at ../../src/bfd/coffcode.h line 842 in handle_COMDAT

 BFD: Please report this bug.

This is with stock gcc and binutils.  Looks like something
broke recently on the binutils/bfd side...

> Thus I would like to get approval for the following patch,
> ChangeLog entry:
> 
> 2008-10-02  Pierre Muller  <muller@ics.u-strasbg.fr>
> 		Pedro Alves  <pedro@codesourcery.com>
> 
> 	* win32-nat.c (do_initial_win32_stuff): Set inferior_ptid.

We're doing a small adjustment that I should have made myself,
due to new core expectations, so I don't think we don't need to
bother Christopher with this one.

OK.  Please check it in.

Thanks!

-- 
Pedro Alves

RE: [RFA] fix win32-nat failure

[Pierre Muller]

> > Thus I would like to get approval for the following patch,
> > ChangeLog entry:
> >
> > 2008-10-02  Pierre Muller  <muller@ics.u-strasbg.fr>
> > 		Pedro Alves  <pedro@codesourcery.com>
> >
> > 	* win32-nat.c (do_initial_win32_stuff): Set inferior_ptid.
> 
> We're doing a small adjustment that I should have made myself,
> due to new core expectations, so I don't think we don't need to
> bother Christopher with this one.
> 
> OK.  Please check it in.

Done!
 


Pierre Muller
Pascal language support maintainer for GDB

Re: [RFA] fix win32-nat failure

[Christopher Faylor]

On Thu, Oct 02, 2008 at 02:01:20PM +0100, Pedro Alves wrote:
>OK.  Please check it in.

Um.  Wait a minute.  You may be a global maintainer now but as far as I
understand the process that doesn't give you carte blanche to make
changes in win32-nat.c or other parts of the code which have
maintainers.

I've been following this discussion to see the outcome and I have no
problems with the fix but I don't want you to assume that you have the
blanket right to authorize changes to win32-nat.c unless I seem to be
unresponsive for some period of time.

cgf

Re: [RFA] fix win32-nat failure

[Pedro Alves]

On Thursday 02 October 2008 16:25:00, Christopher Faylor wrote:
> On Thu, Oct 02, 2008 at 02:01:20PM +0100, Pedro Alves wrote:
> >OK.  Please check it in.
> 
> Um.  Wait a minute.  You may be a global maintainer now but as far as I
> understand the process that doesn't give you carte blanche to make
> changes in win32-nat.c or other parts of the code which have
> maintainers.

I understand that.  I'm sorry for if it sounded I was bypassing the process.
I just considered that this was an uncontroversial change in the light
of what was expected from the target, and that it almost didn't require
any win32 expertise at all.

> I've been following this discussion to see the outcome and I have no
> problems with the fix but I don't want you to assume that you have the
> blanket right to authorize changes to win32-nat.c unless I seem to be
> unresponsive for some period of time.

I didn't assume that.  Again, sorry if it sounded like so.  I was really
trying to *avoid* giving you extra trouble, due to an obvious crash I
myself introduced.  I'll certainly be more careful next time.

-- 
Pedro Alves

Re: [RFA] fix win32-nat failure

[Christopher Faylor]

On Thu, Oct 02, 2008 at 04:54:09PM +0100, Pedro Alves wrote:
>On Thursday 02 October 2008 16:25:00, Christopher Faylor wrote:
>>On Thu, Oct 02, 2008 at 02:01:20PM +0100, Pedro Alves wrote:
>>>OK.  Please check it in.
>>
>>Um.  Wait a minute.  You may be a global maintainer now but as far as I
>>understand the process that doesn't give you carte blanche to make
>>changes in win32-nat.c or other parts of the code which have
>>maintainers.
>
>I understand that.  I'm sorry for if it sounded I was bypassing the
>process.  I just considered that this was an uncontroversial change in
>the light of what was expected from the target, and that it almost
>didn't require any win32 expertise at all.
>
>>I've been following this discussion to see the outcome and I have no
>>problems with the fix but I don't want you to assume that you have the
>>blanket right to authorize changes to win32-nat.c unless I seem to be
>>unresponsive for some period of time.
>
>I didn't assume that.  Again, sorry if it sounded like so.  I was
>really trying to *avoid* giving you extra trouble, due to an obvious
>crash I myself introduced.  I'll certainly be more careful next time.

Ok.  Understood.  Sorry if the above came across harsh.  I was surprised
and should not responded in such a knee-jerk fashion.

Thank you for being responsive.  When I have time I will look into the
other problems that you noticed, too, unless you already have tracked
those down.

cgf