[MI] Duplicate --thread-group flag not detected

[MI] Duplicate --thread-group flag not detected

[Marc Khouzam]

Hi,

There is a missing 'else' in mi_parse() which prevents the
detection of a duplicate --thread-group flag.

> ./gdb
GNU gdb (GDB) 7.2.50.20101125-cvs
(gdb) interpreter-exec mi "-break-insert --thread-group i1 --thread-group i1 main"
^error,msg="mi_cmd_break_insert: Unknown option ``-thread-group''"

instead of giving an error about "Duplicate --thread-group flag".

No regressions.

Ok?
What about the 7_2 branch?

2010-11-25  Marc Khouzam  <marc.khouzam@ericsson.com>

	* mi/mi-parse.c (mi_parse): Missing else.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/mi/mi-parse.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.c,v
retrieving revision 1.21
diff -u -r1.21 mi-parse.c
--- gdb/mi/mi-parse.c   17 May 2010 20:49:39 -0000      1.21
+++ gdb/mi/mi-parse.c   25 Nov 2010 20:43:31 -0000
@@ -319,7 +319,7 @@
          chp += 1;
          parse->thread_group = strtol (chp, &chp, 10);
        }
-      if (strncmp (chp, "--thread ", ts) == 0)
+      else if (strncmp (chp, "--thread ", ts) == 0)
        {
          if (parse->thread != -1)
            error (_("Duplicate '--thread' option"));

Re: [MI] Duplicate --thread-group flag not detected

[Joel Brobecker]

> 2010-11-25  Marc Khouzam  <marc.khouzam@ericsson.com>
> 
> 	* mi/mi-parse.c (mi_parse): Missing else.

Normally, the man to approve this is Vladimir, so I can't approve, but
I had a quick look anyway.

I think that the patch above is correct, given the current implementation.
Another very bad scenario:

        <mi-command> --thread-group i1 --all --frame 1 --thread 1

I think that all the options after --thread-group will be ignored.

Speaking of the current implementation:

The whole parsing loop looked very odd at first, but I think I'm starting
to understand how it works, now.  I think it does work right now because
there is only 1 option that takes no argument ("--all").  So the code
'swallows' it first if found, and then checks for the other possible
switches.  It apparently tries to avoid error-handling duplication by
putting it at the end of the looping block, but then that leads to:

      if (*chp != '\0' && !isspace (*chp))
        error (_("Invalid value for the '%s' option"),
               start[2] == 't' ? "--thread" : "--frame");

... which is missing one case: --thread-group. As the number of options
being handled grows (for instance, I have in my TODO an entry for adding
support for "--task"), I think that we're going to have to rewrite this
slightly differently to make it easier to read and extend...

Another remark: It looks like the use of the --all/--thread-group/--thread
options with the -mi-break command is not documented? I could only see
them documented with commands such as -exec-continue....

Last comment: A small test would be nice...

-- 
Joel

Re: [MI] Duplicate --thread-group flag not detected

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> 2010-11-25  Marc Khouzam  <marc.khouzam@ericsson.com>
>> 
>> * mi/mi-parse.c (mi_parse): Missing else.

Joel> Normally, the man to approve this is Vladimir, so I can't approve, but
Joel> I had a quick look anyway.

I think in this case the patch is simple and obvious enough that we can
go forward without waiting.

What do you think of that?

Joel> Another remark: It looks like the use of the --all/--thread-group/--thread
Joel> options with the -mi-break command is not documented? I could only see
Joel> them documented with commands such as -exec-continue....

This code seems weird to me too.  I suppose it is needed until the
transition to "new" style argv commands is completed.

I was curious about this -- is this sort of transition something people
would want to see done?  Would it help MI users?  (It would help Python
if we ever implemented the automatic MI wrapping... but I am not sure
whether we really want to do that.)

From what I can see there are 14 such commands.

Joel> Last comment: A small test would be nice...

I agree.

Tom

Re: [MI] Duplicate --thread-group flag not detected

[Joel Brobecker]

> I think in this case the patch is simple and obvious enough that we can
> go forward without waiting.
> 
> What do you think of that?

Agreed. I am also OK for the 7.2 branch, even if it's not exactly
a critical bug.

> I was curious about this -- is this sort of transition something people
> would want to see done?  Would it help MI users?  (It would help Python
> if we ever implemented the automatic MI wrapping... but I am not sure
> whether we really want to do that.)

I hope others have enough experience with MI that we can have some
useful answers. I don't have much experience with MI myself...


-- 
Joel

RE: [MI] Duplicate --thread-group flag not detected

[Marc Khouzam]

> > I think in this case the patch is simple and obvious enough that we can
> > go forward without waiting.
> >
> > What do you think of that?
> 
> Agreed. I am also OK for the 7.2 branch, even if it's not exactly
> a critical bug.

So, I can commit to both branches?  Even without the new test?
I apologize, but I just don't have time to write new tests, because I know
that if I start, I'll make sure every case is tested, and that will take
more time than I have for a bug that is not really that important.

I'll wait for you go/no-go to commit.

> > I was curious about this -- is this sort of transition something people
> > would want to see done?  Would it help MI users?  (It would help Python
> > if we ever implemented the automatic MI wrapping... but I am not sure
> > whether we really want to do that.)
> 
> I hope others have enough experience with MI that we can have some
> useful answers. I don't have much experience with MI myself...

I'm personally not familiar with what you mean with "new style argv commands",
so I can't and anything to that.


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

	* mi/mi-parse.c (mi_parse): Missing else.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/mi/mi-parse.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.c,v
retrieving revision 1.21
diff -u -r1.21 mi-parse.c
--- gdb/mi/mi-parse.c   17 May 2010 20:49:39 -0000      1.21
+++ gdb/mi/mi-parse.c   25 Nov 2010 20:43:31 -0000
@@ -319,7 +319,7 @@
          chp += 1;
          parse->thread_group = strtol (chp, &chp, 10);
        }
-      if (strncmp (chp, "--thread ", ts) == 0)
+      else if (strncmp (chp, "--thread ", ts) == 0)
        {
          if (parse->thread != -1)
            error (_("Duplicate '--thread' option"));

Re: [MI] Duplicate --thread-group flag not detected

[Joel Brobecker]

> > Agreed. I am also OK for the 7.2 branch, even if it's not exactly
> > a critical bug.
> 
> So, I can commit to both branches?  Even without the new test?
> I apologize, but I just don't have time to write new tests, because I know
> that if I start, I'll make sure every case is tested, and that will take
> more time than I have for a bug that is not really that important.

Yes, please go ahead.

Too bad for the testcase, as this means that we can potentially re-introduce
the bug later on. One thing I had to learn early in my career was to not let
best be the enemy of good. One test is better than none, so if you can't
cover all cases but have enough time to cover one, then one is always better
than none.

-- 
Joel

RE: [MI] Duplicate --thread-group flag not detected

[Marc Khouzam]

> > > Agreed. I am also OK for the 7.2 branch, even if it's not exactly
> > > a critical bug.
> >
> > So, I can commit to both branches?  Even without the new test?
> > I apologize, but I just don't have time to write new tests, because I know
> > that if I start, I'll make sure every case is tested, and that will take
> > more time than I have for a bug that is not really that important.
> 
> Yes, please go ahead.
> 
> Too bad for the testcase, as this means that we can potentially re-introduce
> the bug later on. One thing I had to learn early in my career was to not let
> best be the enemy of good. One test is better than none, so if you can't
> cover all cases but have enough time to cover one, then one is always better
> than none.

You make it hard to say no :-)

So here's my attempt at new tests for these cases.  I wasn't sure where to put
it so I created a new mi-general.exp test which should eventually test the parsing
of general MI syntax, including the --thread-group, --thread, --frame, --all, --reverse,
flags.

The problem is that there is another bug that makes the new tests fail, so I had to
temporarily comment them out:
http://sourceware.org/ml/gdb-patches/2010-11/msg00436.html

Once this second bug fix is approved, I'll un-comment the tests and make sure they
pass.

So, I'm re-submitting this patch for approval, so that you guys can comment on the tests.

Thanks

Marc

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

	* mi/mi-parse.c (mi_parse): Missing else.
        

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

	* gdb.mi/mi-general.exp: New file.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/mi/mi-parse.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.c,v
retrieving revision 1.21
diff -u -r1.21 mi-parse.c
--- gdb/mi/mi-parse.c   17 May 2010 20:49:39 -0000      1.21
+++ gdb/mi/mi-parse.c   5 Dec 2010 19:07:26 -0000
@@ -319,7 +319,7 @@
          chp += 1;
          parse->thread_group = strtol (chp, &chp, 10);
        }
-      if (strncmp (chp, "--thread ", ts) == 0)
+      else if (strncmp (chp, "--thread ", ts) == 0)
        {
          if (parse->thread != -1)
            error (_("Duplicate '--thread' option"));
Index: gdb/testsuite/gdb.mi/mi-general.exp
===================================================================
RCS file: gdb/testsuite/gdb.mi/mi-general.exp
diff -N gdb/testsuite/gdb.mi/mi-general.exp
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.mi/mi-general.exp 1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,67 @@
+# Copyright 1999, 2000, 2001, 2002, 2003, 2005, 2007, 2008, 2009, 2010
+# Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+#
+# test general features of Machine interface (MI) operations
+#
+# Verify that, the special MI flags --thread-group, --thread, --frame,
+# --all, --reverse, are parsed properly.
+# The goal is not to test gdb functionality, which is done by other tests,
+# but the command syntax and correct output response to MI operations.
+#
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start separate-inferior-tty] {
+    continue
+}
+
+set testfile "basics"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/mi-basics
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
+     untested mi-general.exp
+     return -1
+}
+
+mi_run_to_main
+
+proc test_thread_group_flag {} {
+
+    # If we get to the breakpoint error, it means the -thread-group was parsed properly    
+    mi_gdb_test "18-break-insert --thread-group i1 bogus" \
+             "18\\^error,msg=\"Function \\\\\"bogus\\\\\" not defined.\"" \
+             "Valid --thread-group flag"
+
+# This test fails because of improper MI error output 
+#    mi_gdb_test "21-break-insert --thread-group iX main" \
+#             "21\\^error,msg=\"Invalid thread group for the --thread-group option\""
+#             "Invalid --thread-group flag"
+
+# This test fails because of improper MI error output 
+#    mi_gdb_test "36-break-insert --thread-group i1 --thread-group i2 main" \
+#             "36\\^error,msg=\"Duplicate '--thread-group' option\n\"" \
+#             "Duplicate --thread-group flag"
+}
+
+
+test_thread_group_flag
+
+mi_gdb_exit
+return 0

Re: [MI] Duplicate --thread-group flag not detected

[Joel Brobecker]

> You make it hard to say no :-)

:-D

> So here's my attempt at new tests for these cases.  I wasn't sure
> where to put it so I created a new mi-general.exp test which should
> eventually test the parsing of general MI syntax, including the
> --thread-group, --thread, --frame, --all, --reverse, flags.

OK. I propose we treat this patch as separate. Please just go ahead
with the code part, while we review the testing part.

> The problem is that there is another bug that makes the new tests
> fail, so I had to temporarily comment them out:
> http://sourceware.org/ml/gdb-patches/2010-11/msg00436.html

Let's not comment them out, but mark them KFAIL instead, if we can.


> 2010-12-05  Marc Khouzam  <marc.khouzam@ericsson.com>
> 
> 	* gdb.mi/mi-general.exp: New file.

Overal, the test seems fine, but I have limited experimence with
MI testcases.

> +set testfile "basics"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/mi-basics

I believe we decided in the past that it was a bad idea to share the same
example program between testcases. One reason is that, if we split the
gdb.mi testcase in multiple groups to allow more parallel testing, we
might run into troubles.  However, I am just saying that for your info.
It's not your problem, and I know that you have little time for this
testcase, and I already appreciate the effort you put into it. So it's
fine to leave it as is.

> +    mi_gdb_test "18-break-insert --thread-group i1 bogus" \
> +             "18\\^error,msg=\"Function \\\\\"bogus\\\\\" not defined.\"" \
> +             "Valid --thread-group flag"

Do we want to test the MI command with a sequence number. I know that
they are allowed, but aren't they obsolete?

Other than that, I don't have any other recommendation.

-- 
Joel

RE: [MI] Duplicate --thread-group flag not detected

[Marc Khouzam]

 

> -----Original Message-----
> From: Joel Brobecker [mailto:brobecker@adacore.com] 
> Sent: Monday, December 06, 2010 6:04 AM
> To: Marc Khouzam
> Cc: Tom Tromey; 'gdb-patches@sourceware.org'
> Subject: Re: [MI] Duplicate --thread-group flag not detected
> 
> > You make it hard to say no :-)
> 
> :-D
> 
> > So here's my attempt at new tests for these cases.  I wasn't sure
> > where to put it so I created a new mi-general.exp test which should
> > eventually test the parsing of general MI syntax, including the
> > --thread-group, --thread, --frame, --all, --reverse, flags.
> 
> OK. I propose we treat this patch as separate. Please just go ahead
> with the code part, while we review the testing part.

I'll email separately for the testcase then.

I committed the code part to HEAD and 7_2

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

	* mi/mi-parse.c (mi_parse): Missing else.
        
### Eclipse Workspace Patch 1.0
#P src
Index: gdb/mi/mi-parse.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.c,v
retrieving revision 1.21
diff -u -r1.21 mi-parse.c
--- gdb/mi/mi-parse.c   17 May 2010 20:49:39 -0000      1.21
+++ gdb/mi/mi-parse.c   5 Dec 2010 19:07:26 -0000
@@ -319,7 +319,7 @@
          chp += 1;
          parse->thread_group = strtol (chp, &chp, 10);
        }
-      if (strncmp (chp, "--thread ", ts) == 0)
+      else if (strncmp (chp, "--thread ", ts) == 0)
        {
          if (parse->thread != -1)
            error (_("Duplicate '--thread' option"));

[MI] --thread-group test (was: RE: [MI] Duplicate --thread-group flag not detected)

[Marc Khouzam]

 
> > The problem is that there is another bug that makes the new tests
> > fail, so I had to temporarily comment them out:
> > http://sourceware.org/ml/gdb-patches/2010-11/msg00436.html
> 
> Let's not comment them out, but mark them KFAIL instead, if we can.

Do I need a PR number to use KFAIL?  I don't have one.
I used XFAIL instead.  Is that right?
The annoying part there, is that the tests fails on a timeout,
so marking them XFAIL instead of commenting them out makes
the test file take 10 seconds more...

> > 2010-12-05  Marc Khouzam  <marc.khouzam@ericsson.com>
> > 
> > 	* gdb.mi/mi-general.exp: New file.
> 
> Overal, the test seems fine, but I have limited experimence with
> MI testcases.
> 
> > +set testfile "basics"
> > +set srcfile ${testfile}.c
> > +set binfile ${objdir}/${subdir}/mi-basics
> 
> I believe we decided in the past that it was a bad idea to 
> share the same
> example program between testcases. One reason is that, if we split the
> gdb.mi testcase in multiple groups to allow more parallel testing, we
> might run into troubles.  However, I am just saying that for 
> your info.
> It's not your problem, and I know that you have little time for this
> testcase, and I already appreciate the effort you put into it. So it's
> fine to leave it as is.

To be honest, I don't even need a binary for this test, I can test the 
syntax without actually starting the inferior.  I did this in the patch
below, but I'm not sure if this follows proper 'protocol'.  Just let me
know.

> > +    mi_gdb_test "18-break-insert --thread-group i1 bogus" \
> > +             "18\\^error,msg=\"Function \\\\\"bogus\\\\\" 
> not defined.\"" \
> > +             "Valid --thread-group flag"
> 
> Do we want to test the MI command with a sequence number. I know that
> they are allowed, but aren't they obsolete?

We heavily use them in Eclipse :-)
And I saw that other tests use them, so I thought it would make a
'syntax' test more complete.

Thanks

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

	* gdb.mi/mi-general.exp: New file.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/testsuite/gdb.mi/mi-general.exp
===================================================================
RCS file: gdb/testsuite/gdb.mi/mi-general.exp
diff -N gdb/testsuite/gdb.mi/mi-general.exp
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.mi/mi-general.exp 1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,67 @@
+# Copyright 1999, 2000, 2001, 2002, 2003, 2005, 2007, 2008, 2009, 2010
+# Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+#
+# test general features of Machine interface (MI) operations
+#
+# Verify that, the special MI flags --thread-group, --thread, --frame,
+# --all, --reverse, are parsed properly.
+# The goal is not to test gdb functionality, which is done by other tests,
+# but the command syntax and correct output response to MI operations.
+#
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start separate-inferior-tty] {
+    continue
+}
+
+# No need to actually start the inferior as the tests below only verify MI syntax.
+# What we do instead is see that the parsing works, and passes the MI command
+# further down to GDB, which will then fail because we didn't start the inferior.
+# We wouldn't make it that far if the MI syntax parsing failed
+
+proc test_thread_group_flag {} {
+
+    # If we get to the breakpoint error, it means the -thread-group was parsed properly    
+    mi_gdb_test "18-break-insert --thread-group i1 bogus" \
+             "18\\^error,msg=\"No symbol table is loaded.  Use the \\\\\"file\\\\\" command.\"" \
+             "Valid --thread-group flag"
+
+    # Invalid threadGroupId but it starts with an 'i'.  This is a different code path
+    # than an invalid threadGroupId that does not start with an 'i'.  See below test.
+    mi_gdb_test "21-break-insert --thread-group iX main" \
+             "21\\^error,msg=\"Invalid thread group for the --thread-group option\"" \
+             "Invalid --thread-group flag which starts with an i"
+
+    setup_xfail *-*-*
+    mi_gdb_test "25-break-insert --thread-group XYZ main" \
+             "25\\^error,msg=\"Invalid thread group id\"" \
+             "Invalid --thread-group flag which does not start with an i"
+
+    setup_xfail *-*-*
+    mi_gdb_test "36-break-insert --thread-group i1 --thread-group i2 main" \
+             "36\\^error,msg=\"Duplicate '--thread-group' option\n\"" \
+             "Duplicate --thread-group flag"
+}
+
+
+test_thread_group_flag
+
+mi_gdb_exit
+return 0

Re: [MI] --thread-group test (was: RE: [MI] Duplicate --thread-group flag not detected)

[Joel Brobecker]

> Do I need a PR number to use KFAIL?  I don't have one.
> I used XFAIL instead.  Is that right?

XFAIL is for the case where the problem is caused by what I call
the "environment".  In other words, it's not GDB's fault. It could
be bad debugging info, kernel bug, assember, etc...

It took me a while to find the documentation for setup_kfail, but
I confirm that bug ID must be provided with setup_kfail.

> The annoying part there, is that the tests fails on a timeout,
> so marking them XFAIL instead of commenting them out makes
> the test file take 10 seconds more...

In that case, I suggest we simply open bugs for these tests. I understand 
you are already making an effort trying to contribute this test, so I can
create the new PRs for you if you are busy.  In the meantime, let's take
these tests out.

> To be honest, I don't even need a binary for this test, I can test the 
> syntax without actually starting the inferior.  I did this in the patch
> below, but I'm not sure if this follows proper 'protocol'.  Just let me
> know.

I would be perfectly fine.

> > Do we want to test the MI command with a sequence number. I know that
> > they are allowed, but aren't they obsolete?
> 
> We heavily use them in Eclipse :-)
> And I saw that other tests use them, so I thought it would make a
> 'syntax' test more complete.

In that case, that's also a good idea.

> 2010-12-06  Marc Khouzam  <marc.khouzam@ericsson.com>
> 
> 	* gdb.mi/mi-general.exp: New file.

> +# No need to actually start the inferior as the tests below only verify MI syntax.
> +# What we do instead is see that the parsing works, and passes the MI command
> +# further down to GDB, which will then fail because we didn't start the inferior.
> +# We wouldn't make it that far if the MI syntax parsing failed

Thanks a lot for writing comments - I think they are the best feature
of any language and always find them very helpful.

Just watch out for line length, which should not exceed 78 characters.
Also, a period at the end of the last sentence is missing.

> +    # If we get to the breakpoint error, it means the -thread-group was parsed properly    

Same here... and later on as well.

> +    setup_xfail *-*-*
> +    mi_gdb_test "25-break-insert --thread-group XYZ main" \
> +             "25\\^error,msg=\"Invalid thread group id\"" \
> +             "Invalid --thread-group flag which does not start with an i"
> +
> +    setup_xfail *-*-*
> +    mi_gdb_test "36-break-insert --thread-group i1 --thread-group i2 main" \
> +             "36\\^error,msg=\"Duplicate '--thread-group' option\n\"" \
> +             "Duplicate --thread-group flag"

So, as discussed above, let's discard these in favor of opening
new PRs.

Thanks again for doing this,
-- 
Joel