RE: segfault: what should happen when I remove an inferior that is running?

RE: segfault: what should happen when I remove an inferior that is running?

[Marc Khouzam]

From: Pedro Alves [pedro@codesourcery.com]
Sent: December 14, 2010 10:23 AM

> On Tuesday 14 December 2010 15:08:31, Tom Tromey wrote:
> >      Removes the inferior INFNO.  It is not possible to remove an
> >      inferior that is running with this command.
> >
> > I think this means we are missing an error check somewhere.
> 
> Yeah.  I guess we could also offer to kill it or some such,
> but I guess I must have thought that it'd be simpler to
> just error out.  (either way, the user needs to take further
> action, so erroring out is simpler.)

This patch errors out for 'remove-inferior' and '-remove-inferior'
if the inferior still has a pid.

No regressions.

If it is ok, please let me know if you want it only in HEAD or
also in 7_2.

Thanks

Marc

2010-12-17  Marc Khouzam  <marc.khouzam@ericsson.com>

        * inferior.c (remove_inferior_command): Don't remove a running inferior.
	* mi/mi-main.c (mi_cmd_remove_inferior): Ditto.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/inferior.c
===================================================================
RCS file: /cvs/src/src/gdb/inferior.c,v
retrieving revision 1.20
diff -u -r1.20 inferior.c
--- gdb/inferior.c      28 Nov 2010 04:31:24 -0000      1.20
+++ gdb/inferior.c      18 Dec 2010 02:39:50 -0000
@@ -755,6 +755,9 @@
   if (inf == current_inferior ())
     error (_("Can not remove current symbol inferior."));
 
+  if (inf->pid != 0)
+    error (_("Can not remove a running inferior."));
+
   delete_inferior_1 (inf, 1);
 }
 
Index: gdb/mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.184
diff -u -r1.184 mi-main.c
--- gdb/mi/mi-main.c    18 Dec 2010 02:10:05 -0000      1.184
+++ gdb/mi/mi-main.c    18 Dec 2010 02:39:50 -0000
@@ -1772,6 +1772,9 @@
   if (!inf)
     error ("the specified thread group does not exist");
 
+  if (inf->pid != 0)
+    error ("can not remove a running inferior");
+
   if (inf == current_inferior ())
     {
       struct thread_info *tp = 0;

Re: segfault: what should happen when I remove an inferior that is running?

[Pedro Alves]

On Saturday 18 December 2010 02:46:39, Marc Khouzam wrote:
> From: Pedro Alves [pedro@codesourcery.com]
> Sent: December 14, 2010 10:23 AM
> 
> > On Tuesday 14 December 2010 15:08:31, Tom Tromey wrote:
> > >      Removes the inferior INFNO.  It is not possible to remove an
> > >      inferior that is running with this command.
> > >
> > > I think this means we are missing an error check somewhere.
> > 
> > Yeah.  I guess we could also offer to kill it or some such,
> > but I guess I must have thought that it'd be simpler to
> > just error out.  (either way, the user needs to take further
> > action, so erroring out is simpler.)
> 
> This patch errors out for 'remove-inferior' and '-remove-inferior'
> if the inferior still has a pid.
> 
> No regressions.
> 
> If it is ok, please let me know if you want it only in HEAD or
> also in 7_2.

"running inferior" will sound a bit odd for cores, but,
since I don't have a better suggestion that wouldn't involve
more coding, this is okay with me (HEAD and 7_2).

> 
> Thanks
> 
> Marc
> 
> 2010-12-17  Marc Khouzam  <marc.khouzam@ericsson.com>
> 
>         * inferior.c (remove_inferior_command): Don't remove a running inferior.
> 	* mi/mi-main.c (mi_cmd_remove_inferior): Ditto.
> 
> ### Eclipse Workspace Patch 1.0
> #P src
> Index: gdb/inferior.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/inferior.c,v
> retrieving revision 1.20
> diff -u -r1.20 inferior.c
> --- gdb/inferior.c      28 Nov 2010 04:31:24 -0000      1.20
> +++ gdb/inferior.c      18 Dec 2010 02:39:50 -0000
> @@ -755,6 +755,9 @@
>    if (inf == current_inferior ())
>      error (_("Can not remove current symbol inferior."));
>  
> +  if (inf->pid != 0)
> +    error (_("Can not remove a running inferior."));
> +
>    delete_inferior_1 (inf, 1);
>  }
>  
> Index: gdb/mi/mi-main.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
> retrieving revision 1.184
> diff -u -r1.184 mi-main.c
> --- gdb/mi/mi-main.c    18 Dec 2010 02:10:05 -0000      1.184
> +++ gdb/mi/mi-main.c    18 Dec 2010 02:39:50 -0000
> @@ -1772,6 +1772,9 @@
>    if (!inf)
>      error ("the specified thread group does not exist");
>  
> +  if (inf->pid != 0)
> +    error ("can not remove a running inferior");
> +
>    if (inf == current_inferior ())
>      {
>        struct thread_info *tp = 0;
> 

RE: segfault: what should happen when I remove an inferior that is running?

[Marc Khouzam]

 

> -----Original Message-----
> From: Pedro Alves [mailto:pedro@codesourcery.com] 
> Sent: Wednesday, December 22, 2010 7:10 AM
> To: Marc Khouzam
> Cc: Tom Tromey; gdb-patches@sourceware.org
> Subject: Re: segfault: what should happen when I remove an 
> inferior that is running?
> 
> On Saturday 18 December 2010 02:46:39, Marc Khouzam wrote:
> > From: Pedro Alves [pedro@codesourcery.com]
> > Sent: December 14, 2010 10:23 AM
> > 
> > > On Tuesday 14 December 2010 15:08:31, Tom Tromey wrote:
> > > >      Removes the inferior INFNO.  It is not possible to 
> remove an
> > > >      inferior that is running with this command.
> > > >
> > > > I think this means we are missing an error check somewhere.
> > > 
> > > Yeah.  I guess we could also offer to kill it or some such,
> > > but I guess I must have thought that it'd be simpler to
> > > just error out.  (either way, the user needs to take further
> > > action, so erroring out is simpler.)
> > 
> > This patch errors out for 'remove-inferior' and '-remove-inferior'
> > if the inferior still has a pid.
> > 
> > No regressions.
> > 
> > If it is ok, please let me know if you want it only in HEAD or
> > also in 7_2.
> 
> "running inferior" will sound a bit odd for cores, but,
> since I don't have a better suggestion that wouldn't involve
> more coding, this is okay with me (HEAD and 7_2).

Would "Can not remove a an active inferior." be better?

Marc


> > 
> > 2010-12-17  Marc Khouzam  <marc.khouzam@ericsson.com>
> > 
> >         * inferior.c (remove_inferior_command): Don't 
> remove a running inferior.
> > 	* mi/mi-main.c (mi_cmd_remove_inferior): Ditto.
> > 
> > ### Eclipse Workspace Patch 1.0
> > #P src
> > Index: gdb/inferior.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/inferior.c,v
> > retrieving revision 1.20
> > diff -u -r1.20 inferior.c
> > --- gdb/inferior.c      28 Nov 2010 04:31:24 -0000      1.20
> > +++ gdb/inferior.c      18 Dec 2010 02:39:50 -0000
> > @@ -755,6 +755,9 @@
> >    if (inf == current_inferior ())
> >      error (_("Can not remove current symbol inferior."));
> >  
> > +  if (inf->pid != 0)
> > +    error (_("Can not remove a running inferior."));
> > +
> >    delete_inferior_1 (inf, 1);
> >  }
> >  
> > Index: gdb/mi/mi-main.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
> > retrieving revision 1.184
> > diff -u -r1.184 mi-main.c
> > --- gdb/mi/mi-main.c    18 Dec 2010 02:10:05 -0000      1.184
> > +++ gdb/mi/mi-main.c    18 Dec 2010 02:39:50 -0000
> > @@ -1772,6 +1772,9 @@
> >    if (!inf)
> >      error ("the specified thread group does not exist");
> >  
> > +  if (inf->pid != 0)
> > +    error ("can not remove a running inferior");
> > +
> >    if (inf == current_inferior ())
> >      {
> >        struct thread_info *tp = 0;
> > 
> 

Re: segfault: what should happen when I remove an inferior that is running?

[Pedro Alves]

On Wednesday 22 December 2010 18:11:45, Marc Khouzam wrote:
> > > If it is ok, please let me know if you want it only in HEAD or
> > > also in 7_2.
> > 
> > "running inferior" will sound a bit odd for cores, but,
> > since I don't have a better suggestion that wouldn't involve
> > more coding, this is okay with me (HEAD and 7_2).
> 
> Would "Can not remove a an active inferior." be better?

Fine with me.

-- 
Pedro Alves

RE: segfault: what should happen when I remove an inferior that is running?

[Marc Khouzam]

> -----Original Message-----
> From: Pedro Alves [mailto:pedro@codesourcery.com] 
> Sent: Wednesday, December 22, 2010 1:17 PM
> To: gdb-patches@sourceware.org
> Cc: Marc Khouzam; 'Tom Tromey'
> Subject: Re: segfault: what should happen when I remove an 
> inferior that is running?
> 
> On Wednesday 22 December 2010 18:11:45, Marc Khouzam wrote:
> > > > If it is ok, please let me know if you want it only in HEAD or
> > > > also in 7_2.
> > > 
> > > "running inferior" will sound a bit odd for cores, but,
> > > since I don't have a better suggestion that wouldn't involve
> > > more coding, this is okay with me (HEAD and 7_2).
> > 
> > Would "Can not remove a an active inferior." be better?
> 
> Fine with me.

I committed the following to 7_2 and HEAD. 

Thanks

Marc

2010-12-22  Marc Khouzam  <marc.khouzam@ericsson.com>

       * inferior.c (remove_inferior_command): Don't remove an active inferior.
       * mi/mi-main.c (mi_cmd_remove_inferior): Ditto.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/inferior.c
===================================================================
RCS file: /cvs/src/src/gdb/inferior.c,v
retrieving revision 1.18
diff -u -r1.18 inferior.c
--- gdb/inferior.c      14 May 2010 21:25:51 -0000      1.18
+++ gdb/inferior.c      22 Dec 2010 18:18:55 -0000
@@ -741,6 +741,9 @@
 
   if (inf == current_inferior ())
     error (_("Can not remove current symbol inferior."));
+    
+  if (inf->pid != 0)
+    error (_("Can not remove an active inferior."));
 
   delete_inferior_1 (inf, 1);
 }
Index: gdb/mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.178.2.5
diff -u -r1.178.2.5 mi-main.c
--- gdb/mi/mi-main.c    18 Dec 2010 01:56:07 -0000      1.178.2.5
+++ gdb/mi/mi-main.c    22 Dec 2010 18:18:55 -0000
@@ -1651,6 +1651,9 @@
   if (!inf)
     error ("the specified thread group does not exist");
 
+  if (inf->pid != 0)
+    error ("can not remove an active inferior");
+
   if (inf == current_inferior ())
     {
       struct thread_info *tp = 0;

Re: segfault: what should happen when I remove an inferior that is running?

[Joel Brobecker]

Mark,

> 2010-12-22  Marc Khouzam  <marc.khouzam@ericsson.com>
> 
>       * inferior.c (remove_inferior_command): Don't remove an active inferior.
>       * mi/mi-main.c (mi_cmd_remove_inferior): Ditto.
[...]
> +  if (inf->pid != 0)
> +    error ("can not remove an active inferior");

Just a small nit that the ARI script discovered, we forgot the i18n
marker:

        error (_("cannot remore an active inferior"));

(I also think that the proper spelling is `cannot' rather than
'can not' - confirmed by wikipedia, even though they say that it is
occasionally spelled as two words)

Would you mind making the obvious fix?

Thanks,
-- 
Joel

Re: segfault: what should happen when I remove an inferior that is running?

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

> Just a small nit that the ARI script discovered, we forgot the i18n
> marker:
> 
>         error (_("cannot remore an active inferior"));
> 
> (I also think that the proper spelling is `cannot' rather than
> 'can not' - confirmed by wikipedia, even though they say that it is
> occasionally spelled as two words)
> 
> Would you mind making the obvious fix?

Looks like Mark is away from the office, so I checked in the attached
fix (HEAD & 7.2 branch)

-- 
Joel

[-- Attachment #2: i18n.diff --]
[-- Type: text/x-diff, Size: 1185 bytes --]

commit f114e5b6d29ff343021bb67765d209fa98d675fb
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Thu Dec 23 08:27:37 2010 +0400

    Add missing _() marker in error message.
    
    gdb/ChangeLog:
    
    	* mi/mi-main.c (mi_cmd_remove_inferior): Use _() marker for error
    	message.  Change spelling of "can not" into "cannot".

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 24db449..4504f11 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2010-12-23  Joel Brobecker  <brobecker@adacore.com>
+
+	* mi/mi-main.c (mi_cmd_remove_inferior): Use _() marker for error
+	message.  Change spelling of "can not" into "cannot".
+
 2010-12-23  Yao Qi  <yao@codesourcery.com>
 
 	* arm-tdep.c (arm_gdbarch_init): Remove invoke to
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 6f66e89..f713412 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1773,7 +1773,7 @@ mi_cmd_remove_inferior (char *command, char **argv, int argc)
     error ("the specified thread group does not exist");
 
   if (inf->pid != 0)
-    error ("can not remove an active inferior");
+    error (_("cannot remove an active inferior"));
 
   if (inf == current_inferior ())
     {

RE: segfault: what should happen when I remove an inferior that is running?

[Marc Khouzam]

> -----Original Message-----
> From: Joel Brobecker [mailto:brobecker@adacore.com] 
> Sent: Wednesday, December 22, 2010 11:38 PM
> To: Marc Khouzam
> Cc: 'Pedro Alves'; 'gdb-patches@sourceware.org'; 'Tom Tromey'
> Subject: Re: segfault: what should happen when I remove an 
> inferior that is running?
> 
> > Just a small nit that the ARI script discovered, we forgot the i18n
> > marker:
> > 
> >         error (_("cannot remore an active inferior"));
> > 
> > (I also think that the proper spelling is `cannot' rather than
> > 'can not' - confirmed by wikipedia, even though they say that it is
> > occasionally spelled as two words)
> > 
> > Would you mind making the obvious fix?
> 
> Looks like Mark is away from the office, so I checked in the attached
> fix (HEAD & 7.2 branch)

Thanks Joel!

So I know for next time, when should this i18n marker be used?  
Should it be used for any user-visible string output?  
There are other instances of error() being called without it 
from mi-main.c.

> \grep error\ \( mi-main.c | \grep -v error\ \(_ | wc -l
38

Re: segfault: what should happen when I remove an inferior that is running?

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> So I know for next time, when should this i18n marker be used?  
Marc> Should it be used for any user-visible string output?  

Yes.

Marc> There are other instances of error() being called without it 
Marc> from mi-main.c.

These are all bugs.

I think the GNU gettext manual has a lot more details, in case you are
interested.

Tom

Re: segfault: what should happen when I remove an inferior that is running?

[Pedro Alves]

On Thursday 06 January 2011 20:17:23, Tom Tromey wrote:
> Marc> There are other instances of error() being called without it 
> Marc> from mi-main.c.
> 
> These are all bugs.
> 

I think that these instances are for example errors when a frontend
uses the wrong MI command syntax.  Have we decided whether these
should be translated?  Not sure who that helps.
If not, maybe we should have a mi_error function, so that the
ARI doesn't complain about them.

-- 
Pedro Alves