* [RFA] windows: do not crash if inferior
@ 2010-01-13 9:25 Tristan Gingold
2010-01-19 9:22 ` Joel Brobecker
0 siblings, 1 reply; 11+ messages in thread
From: Tristan Gingold @ 2010-01-13 9:25 UTC (permalink / raw)
To: gdb-patches ml
Hi,
in case of early exit of the inferior, gdb crashes due to an assertion failure in inferior_thread().
This can happen if the inferior is not able to load a DLL due to a permission issue.
I am not sure that this is the best fix, but at least it avoids the crash.
Tristan.
2010-01-13 gingold <gingold@adacore.com>
* windows-nat.c (do_initial_windows_stuff): Call error() if the
inferior doesn't exist anymore.
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index f91ca32..29a28a9 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1603,6 +1603,11 @@ do_initial_windows_stuff (struct target_ops *ops, DWORD p
{
stop_after_trap = 1;
wait_for_inferior (0);
+
+ /* The inferior may have exited early, due to eg. a DLL load error. */
+ if (ptid_equal (inferior_ptid, null_ptid))
+ error (_("inferior exited early"));
+
tp = inferior_thread ();
if (tp->stop_signal != TARGET_SIGNAL_TRAP)
resume (0, tp->stop_signal);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA] windows: do not crash if inferior
2010-01-13 9:25 [RFA] windows: do not crash if inferior Tristan Gingold
@ 2010-01-19 9:22 ` Joel Brobecker
2010-01-20 16:16 ` Christopher Faylor
2010-01-20 16:41 ` Pedro Alves
0 siblings, 2 replies; 11+ messages in thread
From: Joel Brobecker @ 2010-01-19 9:22 UTC (permalink / raw)
To: Tristan Gingold; +Cc: gdb-patches ml
> 2010-01-13 gingold <gingold@adacore.com>
>
> * windows-nat.c (do_initial_windows_stuff): Call error() if the
> inferior doesn't exist anymore.
Just some thoughts, as Chris usually reviews Windows patches...
I don't see a "generic" way of dealing with this situation. So the type
of approach you took (returning early from do_initial_windows_stuff)
seems to be the only approach I can see. I was initially a little
reluctant about throwing an error: As far as I can tell from the code,
the debugger should have already printed an error message such as
"Inferior exited with code ..." (is that correct?) - and so an extra
"inferior exited early" message could be considered superfluous. However,
if you do not error-out now, core GDB will assume that target_create_inferior
succeeded and thus possibly do something unexpected as well.
Bottom line - I cannot propose a better approach short of revamping
a bit the target_create_inferior routine to add error-handling,
I think the patch is fine.
I would suggest a different wording, to explain that we were trying
to run the program when the error occured.
error (_("cannot run program, inferior exited prematurely during startup"));
--
Joel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA] windows: do not crash if inferior
2010-01-19 9:22 ` Joel Brobecker
@ 2010-01-20 16:16 ` Christopher Faylor
2010-01-20 16:35 ` Pedro Alves
2010-01-20 17:38 ` Christopher Faylor
2010-01-20 16:41 ` Pedro Alves
1 sibling, 2 replies; 11+ messages in thread
From: Christopher Faylor @ 2010-01-20 16:16 UTC (permalink / raw)
To: gdb-patches ml, Tristan Gingold, Joel Brobecker
On Tue, Jan 19, 2010 at 01:22:27PM +0400, Joel Brobecker wrote:
>> 2010-01-13 gingold <gingold@adacore.com>
>>
>> * windows-nat.c (do_initial_windows_stuff): Call error() if the
>> inferior doesn't exist anymore.
>
>Just some thoughts, as Chris usually reviews Windows patches...
>
>I don't see a "generic" way of dealing with this situation. So the type
>of approach you took (returning early from do_initial_windows_stuff)
>seems to be the only approach I can see. I was initially a little
>reluctant about throwing an error: As far as I can tell from the code,
>the debugger should have already printed an error message such as
>"Inferior exited with code ..." (is that correct?) - and so an extra
>"inferior exited early" message could be considered superfluous. However,
>if you do not error-out now, core GDB will assume that target_create_inferior
>succeeded and thus possibly do something unexpected as well.
>
>Bottom line - I cannot propose a better approach short of revamping
>a bit the target_create_inferior routine to add error-handling,
>I think the patch is fine.
>
>I would suggest a different wording, to explain that we were trying
>to run the program when the error occured.
>
> error (_("cannot run program, inferior exited prematurely during startup"));
Does inferior_thread really need the assert()? If not, we could jsut test
tp for NULL.
Otherwise, I agree with Joel's assessment.
cgf
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA] windows: do not crash if inferior
2010-01-20 16:16 ` Christopher Faylor
@ 2010-01-20 16:35 ` Pedro Alves
2010-01-20 17:38 ` Christopher Faylor
1 sibling, 0 replies; 11+ messages in thread
From: Pedro Alves @ 2010-01-20 16:35 UTC (permalink / raw)
To: gdb-patches; +Cc: Christopher Faylor, Tristan Gingold, Joel Brobecker
On Wednesday 20 January 2010 16:15:49, Christopher Faylor wrote:
> Does inferior_thread really need the assert()? If not, we could jsut test
> tp for NULL.
Yes it does.
--
Pedro Alves
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA] windows: do not crash if inferior
2010-01-19 9:22 ` Joel Brobecker
2010-01-20 16:16 ` Christopher Faylor
@ 2010-01-20 16:41 ` Pedro Alves
1 sibling, 0 replies; 11+ messages in thread
From: Pedro Alves @ 2010-01-20 16:41 UTC (permalink / raw)
To: gdb-patches; +Cc: Joel Brobecker, Tristan Gingold
On Tuesday 19 January 2010 09:22:27, Joel Brobecker wrote:
> I don't see a "generic" way of dealing with this situation. So the type
> of approach you took (returning early from do_initial_windows_stuff)
> seems to be the only approach I can see. I was initially a little
> reluctant about throwing an error: As far as I can tell from the code,
> the debugger should have already printed an error message such as
> "Inferior exited with code ..." (is that correct?) - and so an extra
> "inferior exited early" message could be considered superfluous. However,
> if you do not error-out now, core GDB will assume that target_create_inferior
> succeeded and thus possibly do something unexpected as well.
>
> Bottom line - I cannot propose a better approach short of revamping
> a bit the target_create_inferior routine to add error-handling,
> I think the patch is fine.
IMO, the clean and proper solution is to stop using
wait_for_inferior from within target_create_inferior, similarly
to fork-child.c:startup_inferior --- fork-child.c:startup_inferior
handles the similar case of the inferior process exiting early
during startup before being fully properly created.
--
Pedro Alves
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA] windows: do not crash if inferior
2010-01-20 16:16 ` Christopher Faylor
2010-01-20 16:35 ` Pedro Alves
@ 2010-01-20 17:38 ` Christopher Faylor
2010-01-20 18:57 ` Christopher Faylor
2010-01-20 19:12 ` Pedro Alves
1 sibling, 2 replies; 11+ messages in thread
From: Christopher Faylor @ 2010-01-20 17:38 UTC (permalink / raw)
To: gdb-patches ml
On Wed, Jan 20, 2010 at 11:15:49AM -0500, Christopher Faylor wrote:
>On Tue, Jan 19, 2010 at 01:22:27PM +0400, Joel Brobecker wrote:
>>> 2010-01-13 gingold <gingold@adacore.com>
>>>
>>> * windows-nat.c (do_initial_windows_stuff): Call error() if the
>>> inferior doesn't exist anymore.
>>
>>Just some thoughts, as Chris usually reviews Windows patches...
>>
>>I don't see a "generic" way of dealing with this situation. So the type
>>of approach you took (returning early from do_initial_windows_stuff)
>>seems to be the only approach I can see. I was initially a little
>>reluctant about throwing an error: As far as I can tell from the code,
>>the debugger should have already printed an error message such as
>>"Inferior exited with code ..." (is that correct?) - and so an extra
>>"inferior exited early" message could be considered superfluous. However,
>>if you do not error-out now, core GDB will assume that target_create_inferior
>>succeeded and thus possibly do something unexpected as well.
>>
>>Bottom line - I cannot propose a better approach short of revamping
>>a bit the target_create_inferior routine to add error-handling,
>>I think the patch is fine.
>>
>>I would suggest a different wording, to explain that we were trying
>>to run the program when the error occured.
>>
>> error (_("cannot run program, inferior exited prematurely during startup"));
>
>Does inferior_thread really need the assert()? If not, we could jsut test
>tp for NULL.
>
>Otherwise, I agree with Joel's assessment.
Actually, how about something like this instead? I used the same wording as fork-child.c
after seeing Pedro's note.
cgf
Index: windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.196.4.1
diff -d -u -r1.196.4.1 windows-nat.c
--- windows-nat.c 30 Sep 2009 07:40:10 -0000 1.196.4.1
+++ windows-nat.c 20 Jan 2010 17:35:47 -0000
@@ -41,6 +41,7 @@
#include <psapi.h>
#ifdef __CYGWIN__
#include <sys/cygwin.h>
+#include <cygwin/version.h>
#endif
#include <signal.h>
@@ -123,6 +128,8 @@
static uintptr_t dr[8];
static int debug_registers_changed;
static int debug_registers_used;
+
+static int windows_initialization_done;
#define DR6_CLEAR_VALUE 0xffff0ff0
/* The string sent by cygwin when it processes a signal.
@@ -1399,6 +1407,8 @@
(unsigned) current_event.dwProcessId,
(unsigned) current_event.dwThreadId,
"EXIT_PROCESS_DEBUG_EVENT"));
+ if (!windows_initialization_done)
+ error (_("During startup program exited with code 0x%x."), (unsigned int) current_event.u.ExitProcess.dwExitCode);
if (saw_create != 1)
break;
ourstatus->kind = TARGET_WAITKIND_EXITED;
@@ -1597,6 +1607,7 @@
terminal_init_inferior_with_pgrp (pid);
target_terminal_inferior ();
+ windows_initialization_done = 0;
inf->stop_soon = STOP_QUIETLY;
while (1)
{
@@ -1609,6 +1620,7 @@
break;
}
+ windows_initialization_done = 1;
inf->stop_soon = NO_STOP_QUIETLY;
stop_after_trap = 0;
return;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA] windows: do not crash if inferior
2010-01-20 17:38 ` Christopher Faylor
@ 2010-01-20 18:57 ` Christopher Faylor
2010-01-20 19:12 ` Pedro Alves
1 sibling, 0 replies; 11+ messages in thread
From: Christopher Faylor @ 2010-01-20 18:57 UTC (permalink / raw)
To: gdb-patches ml
On Wed, Jan 20, 2010 at 12:37:46PM -0500, Christopher Faylor wrote:
>On Wed, Jan 20, 2010 at 11:15:49AM -0500, Christopher Faylor wrote:
>>On Tue, Jan 19, 2010 at 01:22:27PM +0400, Joel Brobecker wrote:
>>>> 2010-01-13 gingold <gingold@adacore.com>
>>>>
>>>> * windows-nat.c (do_initial_windows_stuff): Call error() if the
>>>> inferior doesn't exist anymore.
>>>
>>>Just some thoughts, as Chris usually reviews Windows patches...
>>>
>>>I don't see a "generic" way of dealing with this situation. So the type
>>>of approach you took (returning early from do_initial_windows_stuff)
>>>seems to be the only approach I can see. I was initially a little
>>>reluctant about throwing an error: As far as I can tell from the code,
>>>the debugger should have already printed an error message such as
>>>"Inferior exited with code ..." (is that correct?) - and so an extra
>>>"inferior exited early" message could be considered superfluous. However,
>>>if you do not error-out now, core GDB will assume that target_create_inferior
>>>succeeded and thus possibly do something unexpected as well.
>>>
>>>Bottom line - I cannot propose a better approach short of revamping
>>>a bit the target_create_inferior routine to add error-handling,
>>>I think the patch is fine.
>>>
>>>I would suggest a different wording, to explain that we were trying
>>>to run the program when the error occured.
>>>
>>> error (_("cannot run program, inferior exited prematurely during startup"));
>>
>>Does inferior_thread really need the assert()? If not, we could jsut test
>>tp for NULL.
>>
>>Otherwise, I agree with Joel's assessment.
>
>Actually, how about something like this instead? I used the same wording as fork-child.c
>after seeing Pedro's note.
>
>cgf
>
>Index: windows-nat.c
>===================================================================
>RCS file: /cvs/src/src/gdb/windows-nat.c,v
>retrieving revision 1.196.4.1
>diff -d -u -r1.196.4.1 windows-nat.c
>--- windows-nat.c 30 Sep 2009 07:40:10 -0000 1.196.4.1
>+++ windows-nat.c 20 Jan 2010 17:35:47 -0000
>@@ -41,6 +41,7 @@
> #include <psapi.h>
> #ifdef __CYGWIN__
> #include <sys/cygwin.h>
>+#include <cygwin/version.h>
^^^^^^^^^^^^^^^^^^^^^^^^^^^
Sorry, that part is not needed.
cgf
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA] windows: do not crash if inferior
2010-01-20 17:38 ` Christopher Faylor
2010-01-20 18:57 ` Christopher Faylor
@ 2010-01-20 19:12 ` Pedro Alves
2010-01-21 17:57 ` Christopher Faylor
1 sibling, 1 reply; 11+ messages in thread
From: Pedro Alves @ 2010-01-20 19:12 UTC (permalink / raw)
To: gdb-patches; +Cc: Christopher Faylor
On Wednesday 20 January 2010 17:37:46, Christopher Faylor wrote:
> Actually, how about something like this instead? I used the same wording as fork-child.c
> after seeing Pedro's note.
> + if (!windows_initialization_done)
> + error (_("During startup program exited with code 0x%x."), (unsigned int) current_event.u.ExitProcess.dwExitCode);
I think you should call target_mourn_inferior before
throwing, to unpush the target_ops, clear inferior_ptid
and delete any thread the OS had already reported, and
maybe other things. You'll also want to call
target_terminal_ours.
--
Pedro Alves
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA] windows: do not crash if inferior
2010-01-20 19:12 ` Pedro Alves
@ 2010-01-21 17:57 ` Christopher Faylor
2010-01-26 15:59 ` Tristan Gingold
0 siblings, 1 reply; 11+ messages in thread
From: Christopher Faylor @ 2010-01-21 17:57 UTC (permalink / raw)
To: gdb-patches
On Wed, Jan 20, 2010 at 07:12:12PM +0000, Pedro Alves wrote:
>On Wednesday 20 January 2010 17:37:46, Christopher Faylor wrote:
>
>> Actually, how about something like this instead? I used the same wording as fork-child.c
>> after seeing Pedro's note.
>
>> + if (!windows_initialization_done)
>> + error (_("During startup program exited with code 0x%x."), (unsigned int) current_event.u.ExitProcess.dwExitCode);
>
>I think you should call target_mourn_inferior before
>throwing, to unpush the target_ops, clear inferior_ptid
>and delete any thread the OS had already reported, and
>maybe other things. You'll also want to call
>target_terminal_ours.
Yes, that should have been obvious to me since I was trying to duplicate
fork-child.c and it does exactly what you suggested. So the new patch
is below.
I don't have an easy way to test this. Can the OP confirm/deny that this
works as intended?
cgf
2010-01-21 Christopher Faylor <me+gdb@cgf.cx>
* windows-nat.c (windows_initialization_done): New variable.
(get_windows_debug_event): Issue error when process dies before
completely initializing.
(do_initial_windows_stuff): Set flag to indicate when we are done with
the initial steps of attaching to the child.
Index: windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.196.4.1
diff -u -p -r1.196.4.1 windows-nat.c
--- windows-nat.c 30 Sep 2009 07:40:10 -0000 1.196.4.1
+++ windows-nat.c 21 Jan 2010 17:54:58 -0000
@@ -123,6 +128,8 @@ enum
static uintptr_t dr[8];
static int debug_registers_changed;
static int debug_registers_used;
+
+static int windows_initialization_done;
#define DR6_CLEAR_VALUE 0xffff0ff0
/* The string sent by cygwin when it processes a signal.
@@ -1399,11 +1407,19 @@ get_windows_debug_event (struct target_o
(unsigned) current_event.dwProcessId,
(unsigned) current_event.dwThreadId,
"EXIT_PROCESS_DEBUG_EVENT"));
- if (saw_create != 1)
- break;
- ourstatus->kind = TARGET_WAITKIND_EXITED;
- ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
- retval = main_thread_id;
+ if (!windows_initialization_done)
+ {
+ target_terminal_ours ();
+ target_mourn_inferior ();
+ error (_("During startup program exited with code 0x%x."),
+ (unsigned int) current_event.u.ExitProcess.dwExitCode);
+ }
+ else if (saw_create == 1)
+ {
+ ourstatus->kind = TARGET_WAITKIND_EXITED;
+ ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
+ retval = main_thread_id;
+ }
break;
case LOAD_DLL_DEBUG_EVENT:
@@ -1597,6 +1613,7 @@ do_initial_windows_stuff (struct target_
terminal_init_inferior_with_pgrp (pid);
target_terminal_inferior ();
+ windows_initialization_done = 0;
inf->stop_soon = STOP_QUIETLY;
while (1)
{
@@ -1609,6 +1626,7 @@ do_initial_windows_stuff (struct target_
break;
}
+ windows_initialization_done = 1;
inf->stop_soon = NO_STOP_QUIETLY;
stop_after_trap = 0;
return;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA] windows: do not crash if inferior
2010-01-21 17:57 ` Christopher Faylor
@ 2010-01-26 15:59 ` Tristan Gingold
2010-01-28 14:55 ` Christopher Faylor
0 siblings, 1 reply; 11+ messages in thread
From: Tristan Gingold @ 2010-01-26 15:59 UTC (permalink / raw)
To: Christopher Faylor; +Cc: gdb-patches
On Jan 21, 2010, at 6:56 PM, Christopher Faylor wrote:
> On Wed, Jan 20, 2010 at 07:12:12PM +0000, Pedro Alves wrote:
>> On Wednesday 20 January 2010 17:37:46, Christopher Faylor wrote:
>>
>>> Actually, how about something like this instead? I used the same wording as fork-child.c
>>> after seeing Pedro's note.
>>
>>> + if (!windows_initialization_done)
>>> + error (_("During startup program exited with code 0x%x."), (unsigned int) current_event.u.ExitProcess.dwExitCode);
>>
>> I think you should call target_mourn_inferior before
>> throwing, to unpush the target_ops, clear inferior_ptid
>> and delete any thread the OS had already reported, and
>> maybe other things. You'll also want to call
>> target_terminal_ours.
>
> Yes, that should have been obvious to me since I was trying to duplicate
> fork-child.c and it does exactly what you suggested. So the new patch
> is below.
>
> I don't have an easy way to test this. Can the OP confirm/deny that this
> works as intended?
Yes, this works as intended.
Tristan.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA] windows: do not crash if inferior
2010-01-26 15:59 ` Tristan Gingold
@ 2010-01-28 14:55 ` Christopher Faylor
0 siblings, 0 replies; 11+ messages in thread
From: Christopher Faylor @ 2010-01-28 14:55 UTC (permalink / raw)
To: gdb-patches, Tristan Gingold
On Tue, Jan 26, 2010 at 04:59:07PM +0100, Tristan Gingold wrote:
>
>On Jan 21, 2010, at 6:56 PM, Christopher Faylor wrote:
>
>> On Wed, Jan 20, 2010 at 07:12:12PM +0000, Pedro Alves wrote:
>>> On Wednesday 20 January 2010 17:37:46, Christopher Faylor wrote:
>>>
>>>> Actually, how about something like this instead? I used the same wording as fork-child.c
>>>> after seeing Pedro's note.
>>>
>>>> + if (!windows_initialization_done)
>>>> + error (_("During startup program exited with code 0x%x."), (unsigned int) current_event.u.ExitProcess.dwExitCode);
>>>
>>> I think you should call target_mourn_inferior before
>>> throwing, to unpush the target_ops, clear inferior_ptid
>>> and delete any thread the OS had already reported, and
>>> maybe other things. You'll also want to call
>>> target_terminal_ours.
>>
>> Yes, that should have been obvious to me since I was trying to duplicate
>> fork-child.c and it does exactly what you suggested. So the new patch
>> is below.
>>
>> I don't have an easy way to test this. Can the OP confirm/deny that this
>> works as intended?
>
>Yes, this works as intended.
Thanks. Applied.
cgf
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-01-28 14:55 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-01-13 9:25 [RFA] windows: do not crash if inferior Tristan Gingold
2010-01-19 9:22 ` Joel Brobecker
2010-01-20 16:16 ` Christopher Faylor
2010-01-20 16:35 ` Pedro Alves
2010-01-20 17:38 ` Christopher Faylor
2010-01-20 18:57 ` Christopher Faylor
2010-01-20 19:12 ` Pedro Alves
2010-01-21 17:57 ` Christopher Faylor
2010-01-26 15:59 ` Tristan Gingold
2010-01-28 14:55 ` Christopher Faylor
2010-01-20 16:41 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox