[PATCH v2] Don't compare types of enum fields

[PATCH v2] Don't compare types of enum fields

[Hannes Domani via Gdb-patches]

Comparing types of enum fields results in a crash, because they don't
have a type.

It can be reproduced by comparing the types of 2 instances of the same
enum type in different objects:

enum.h:
enum e
{
  zero,
  one,
};

enum-1.c:
int func();
enum e e1;
int main()
{
  return e1 + func();
}

enum-2.c:
enum e e2;
int func()
{
  return e2;
}

$ gcc -g -oenum enum-1.c enum-2.c
$ gdb -q enum.exe
Reading symbols from enum.exe...
(gdb) py print(gdb.parse_and_eval("e1").type==gdb.parse_and_eval("e2").type)

Thread 1 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 6184.0x1cc4]
check_typedef (type=0x0) at C:/src/repos/binutils-gdb.git/gdb/gdbtypes.c:2745
2745      while (type->code () == TYPE_CODE_TYPEDEF)

gdb/ChangeLog:

2020-12-17  Hannes Domani  <ssbssa@yahoo.de>

	PR exp/27070
	* gdbtypes.c (check_types_equal): Don't compare types of enum fields.

gdb/testsuite/ChangeLog:

2020-12-17  Hannes Domani  <ssbssa@yahoo.de>

	PR exp/27070
	* gdb.base/pr27070-a.c: New test.
	* gdb.base/pr27070-b.c: New test.
	* gdb.base/pr27070.exp: New file.
	* gdb.base/pr27070.h: New test.
---
v2:
- Add detailed problem description in commit message and test case.
---
 gdb/gdbtypes.c                     |  4 +++-
 gdb/testsuite/gdb.base/pr27070-a.c | 27 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/pr27070-b.c | 25 +++++++++++++++++++++++++
 gdb/testsuite/gdb.base/pr27070.exp | 29 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/pr27070.h   | 22 ++++++++++++++++++++++
 5 files changed, 106 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/pr27070-a.c
 create mode 100644 gdb/testsuite/gdb.base/pr27070-b.c
 create mode 100644 gdb/testsuite/gdb.base/pr27070.exp
 create mode 100644 gdb/testsuite/gdb.base/pr27070.h

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index a40ae5f30e..2207613eef 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -4036,7 +4036,9 @@ check_types_equal (struct type *type1, struct type *type2,
 	    case FIELD_LOC_KIND_ENUMVAL:
 	      if (FIELD_ENUMVAL (*field1) != FIELD_ENUMVAL (*field2))
 		return false;
-	      break;
+	      /* Don't compare types of enum fields, because they don't
+		 have a type.  */
+	      continue;
 	    case FIELD_LOC_KIND_PHYSADDR:
 	      if (FIELD_STATIC_PHYSADDR (*field1)
 		  != FIELD_STATIC_PHYSADDR (*field2))
diff --git a/gdb/testsuite/gdb.base/pr27070-a.c b/gdb/testsuite/gdb.base/pr27070-a.c
new file mode 100644
index 0000000000..f3850803ea
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr27070-a.c
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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/>.  */
+
+#include "pr27070.h"
+
+int func();
+
+enum e e1;
+
+int main()
+{
+  return e1 + func();
+}
diff --git a/gdb/testsuite/gdb.base/pr27070-b.c b/gdb/testsuite/gdb.base/pr27070-b.c
new file mode 100644
index 0000000000..39962c671a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr27070-b.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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/>.  */
+
+#include "pr27070.h"
+
+enum e e2;
+
+int func()
+{
+  return e2;
+}
diff --git a/gdb/testsuite/gdb.base/pr27070.exp b/gdb/testsuite/gdb.base/pr27070.exp
new file mode 100644
index 0000000000..3f565da661
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr27070.exp
@@ -0,0 +1,29 @@
+# Copyright 2020 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/>.
+
+set testname pr27070
+set sources "pr27070-a.c pr27070-b.c"
+
+if {[build_executable ${testname}.exp $testname $sources {debug}] == -1} {
+    return -1
+}
+
+# Start with a fresh gdb.
+
+clean_restart ${testname}
+
+if { [skip_python_tests] } { continue }
+
+gdb_test "py print(gdb.parse_and_eval('e1').type == gdb.parse_and_eval('e2').type)" "True"
diff --git a/gdb/testsuite/gdb.base/pr27070.h b/gdb/testsuite/gdb.base/pr27070.h
new file mode 100644
index 0000000000..f149fc1a7e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr27070.h
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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/>.  */
+
+enum e
+{
+  zero,
+  one,
+};
-- 
2.29.2

Re: [PATCH v2] Don't compare types of enum fields

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

Hannes> 	PR exp/27070
Hannes> 	* gdb.base/pr27070-a.c: New test.
Hannes> 	* gdb.base/pr27070-b.c: New test.
Hannes> 	* gdb.base/pr27070.exp: New file.
Hannes> 	* gdb.base/pr27070.h: New test.

We normally don't name tests after PRs.
I don't recall why this is, and I didn't see it in the test-writing guidelines.
But anyway, I recommend picking a descriptive name instead.

Tom

Re: [PATCH v2] Don't compare types of enum fields

[Simon Marchi via Gdb-patches]

On 2020-12-17 2:29 p.m., Hannes Domani via Gdb-patches wrote:
> Comparing types of enum fields results in a crash, because they don't
> have a type.
> 
> It can be reproduced by comparing the types of 2 instances of the same
> enum type in different objects:
> 
> enum.h:
> enum e
> {
>   zero,
>   one,
> };
> 
> enum-1.c:
> int func();
> enum e e1;
> int main()
> {
>   return e1 + func();
> }
> 
> enum-2.c:
> enum e e2;
> int func()
> {
>   return e2;
> }
> 
> $ gcc -g -oenum enum-1.c enum-2.c
> $ gdb -q enum.exe
> Reading symbols from enum.exe...
> (gdb) py print(gdb.parse_and_eval("e1").type==gdb.parse_and_eval("e2").type)
> 
> Thread 1 received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 6184.0x1cc4]
> check_typedef (type=0x0) at C:/src/repos/binutils-gdb.git/gdb/gdbtypes.c:2745
> 2745      while (type->code () == TYPE_CODE_TYPEDEF)
> 
> gdb/ChangeLog:
> 
> 2020-12-17  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	PR exp/27070
> 	* gdbtypes.c (check_types_equal): Don't compare types of enum fields.
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-12-17  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	PR exp/27070
> 	* gdb.base/pr27070-a.c: New test.
> 	* gdb.base/pr27070-b.c: New test.
> 	* gdb.base/pr27070.exp: New file.
> 	* gdb.base/pr27070.h: New test.
> ---
> v2:
> - Add detailed problem description in commit message and test case.

Same comment as Tom, please find a descriptive name for this test.

When you write the test, it's nice to name it after the PR number because
you don't have to think of a good name.  But that's about it, after that
it doesn't give a clue what the test is about.

> ---
>  gdb/gdbtypes.c                     |  4 +++-
>  gdb/testsuite/gdb.base/pr27070-a.c | 27 +++++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/pr27070-b.c | 25 +++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/pr27070.exp | 29 +++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/pr27070.h   | 22 ++++++++++++++++++++++
>  5 files changed, 106 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.base/pr27070-a.c
>  create mode 100644 gdb/testsuite/gdb.base/pr27070-b.c
>  create mode 100644 gdb/testsuite/gdb.base/pr27070.exp
>  create mode 100644 gdb/testsuite/gdb.base/pr27070.h
> 
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index a40ae5f30e..2207613eef 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -4036,7 +4036,9 @@ check_types_equal (struct type *type1, struct type *type2,
>  	    case FIELD_LOC_KIND_ENUMVAL:
>  	      if (FIELD_ENUMVAL (*field1) != FIELD_ENUMVAL (*field2))
>  		return false;
> -	      break;
> +	      /* Don't compare types of enum fields, because they don't
> +		 have a type.  */
> +	      continue;
>  	    case FIELD_LOC_KIND_PHYSADDR:
>  	      if (FIELD_STATIC_PHYSADDR (*field1)
>  		  != FIELD_STATIC_PHYSADDR (*field2))
> diff --git a/gdb/testsuite/gdb.base/pr27070-a.c b/gdb/testsuite/gdb.base/pr27070-a.c
> new file mode 100644
> index 0000000000..f3850803ea
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr27070-a.c
> @@ -0,0 +1,27 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020 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/>.  */
> +
> +#include "pr27070.h"
> +
> +int func();
> +
> +enum e e1;
> +
> +int main()

int
main (void)

> +{
> +  return e1 + func();
> +}

Space before parens everywhere.

> diff --git a/gdb/testsuite/gdb.base/pr27070-b.c b/gdb/testsuite/gdb.base/pr27070-b.c
> new file mode 100644
> index 0000000000..39962c671a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr27070-b.c
> @@ -0,0 +1,25 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020 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/>.  */
> +
> +#include "pr27070.h"
> +
> +enum e e2;
> +
> +int func()

int
func (void)

> +{
> +  return e2;
> +}

> diff --git a/gdb/testsuite/gdb.base/pr27070.exp b/gdb/testsuite/gdb.base/pr27070.exp
> new file mode 100644
> index 0000000000..3f565da661
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr27070.exp
> @@ -0,0 +1,29 @@
> +# Copyright 2020 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/>.
> +

Please add a short comment explaining what the test tests.  It's useful to refer
to the PR number in that comment, so readers can get more context if they want,
but don't just use "Test for PR 27070.", the comment needs to be informative even
if bugzilla disappears.

> +set testname pr27070
> +set sources "pr27070-a.c pr27070-b.c"

You can probably use:

standard_testfile -a.c -b.c

> +
> +if {[build_executable ${testname}.exp $testname $sources {debug}] == -1} {
> +    return -1
> +}
> +
> +# Start with a fresh gdb.
> +
> +clean_restart ${testname}

You can probably use "prepare_for_testing" which does the build_executable + clean_restart.

> +
> +if { [skip_python_tests] } { continue }

I suppose this can go earlier, to avoid compiling the program and starting GDB if
the test is going to be skipped anyway.

> +
> +gdb_test "py print(gdb.parse_and_eval('e1').type == gdb.parse_and_eval('e2').type)" "True"

I think it would make sense to move this test to the gdb.python directory.

Simon

Re: [PATCH v2] Don't compare types of enum fields

[Hannes Domani via Gdb-patches]

 Am Freitag, 18. Dezember 2020, 22:28:06 MEZ hat Simon Marchi <simon.marchi@polymtl.ca> Folgendes geschrieben:

> On 2020-12-17 2:29 p.m., Hannes Domani via Gdb-patches wrote:
> > +
> > +if { [skip_python_tests] } { continue }
>
> I suppose this can go earlier, to avoid compiling the program and starting GDB if
> the test is going to be skipped anyway.

It fails spectacularly when I try this:

ERROR: tcl error sourcing /c/src/repos/gdb-testsuite/gdb/testsuite/gdb.base/compare-enum-type.exp.
ERROR: can't read "use_gdb_stub": no such variable
    while executing
"if {$use_gdb_stub
    && [regexp -nocase {^\s*(r|run|star|start|at|att|atta|attac|attach)\M}  $command]} {
    error "gdbserver does not support $command wi..."
    (procedure "gdb_test_multiple" line 47)
    invoked from within
"gdb_test_multiple "python print ('test')" "verify python support"  -prompt "$prompt_regexp" {
        -re "not supported.*$prompt_regexp" {
        unsupported..."
    (procedure "skip_python_tests_prompt" line 4)
    invoked from within
"skip_python_tests_prompt "$gdb_prompt $""
    (procedure "skip_python_tests" line 3)
    invoked from within
"skip_python_tests"
    (file "/c/src/repos/gdb-testsuite/gdb/testsuite/gdb.base/compare-enum-type.exp" line 18)
    invoked from within
"source /c/src/repos/gdb-testsuite/gdb/testsuite/gdb.base/compare-enum-type.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source /c/src/repos/gdb-testsuite/gdb/testsuite/gdb.base/compare-enum-type.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
testcase /c/src/repos/gdb-testsuite/gdb/testsuite/gdb.base/compare-enum-type.exp completed in 0 seconds



> > +
> > +gdb_test "py print(gdb.parse_and_eval('e1').type == gdb.parse_and_eval('e2').type)" "True"
>
>
> I think it would make sense to move this test to the gdb.python directory.

I wasn't sure about that, because even though I use python to test it, it's
actually testing check_types_equal (a gdb base function).


Hannes

Re: [PATCH v2] Don't compare types of enum fields

[Simon Marchi via Gdb-patches]

On 2020-12-18 5:00 p.m., Hannes Domani wrote:
>  Am Freitag, 18. Dezember 2020, 22:28:06 MEZ hat Simon Marchi <simon.marchi@polymtl.ca> Folgendes geschrieben:
> 
>> On 2020-12-17 2:29 p.m., Hannes Domani via Gdb-patches wrote:
>>> +
>>> +if { [skip_python_tests] } { continue }
>>
>> I suppose this can go earlier, to avoid compiling the program and starting GDB if
>> the test is going to be skipped anyway.
> 
> It fails spectacularly when I try this:
> 
> ERROR: tcl error sourcing /c/src/repos/gdb-testsuite/gdb/testsuite/gdb.base/compare-enum-type.exp.
> ERROR: can't read "use_gdb_stub": no such variable
>     while executing
> "if {$use_gdb_stub
>     && [regexp -nocase {^\s*(r|run|star|start|at|att|atta|attac|attach)\M}  $command]} {
>     error "gdbserver does not support $command wi..."
>     (procedure "gdb_test_multiple" line 47)
>     invoked from within
> "gdb_test_multiple "python print ('test')" "verify python support"  -prompt "$prompt_regexp" {
>         -re "not supported.*$prompt_regexp" {
>         unsupported..."
>     (procedure "skip_python_tests_prompt" line 4)
>     invoked from within
> "skip_python_tests_prompt "$gdb_prompt $""
>     (procedure "skip_python_tests" line 3)
>     invoked from within
> "skip_python_tests"
>     (file "/c/src/repos/gdb-testsuite/gdb/testsuite/gdb.base/compare-enum-type.exp" line 18)
>     invoked from within
> "source /c/src/repos/gdb-testsuite/gdb/testsuite/gdb.base/compare-enum-type.exp"
>     ("uplevel" body line 1)
>     invoked from within
> "uplevel #0 source /c/src/repos/gdb-testsuite/gdb/testsuite/gdb.base/compare-enum-type.exp"
>     invoked from within
> "catch "uplevel #0 source $test_file_name""
> testcase /c/src/repos/gdb-testsuite/gdb/testsuite/gdb.base/compare-enum-type.exp completed in 0 seconds

Ok, so skip_python_tests requires a started GDB.  That's fine as it is then.

>>> +
>>> +gdb_test "py print(gdb.parse_and_eval('e1').type == gdb.parse_and_eval('e2').type)" "True"
>>
>>
>> I think it would make sense to move this test to the gdb.python directory.
> 
> I wasn't sure about that, because even though I use python to test it, it's
> actually testing check_types_equal (a gdb base function).

Well, it doesn't really matter.  Technically, you are testing the comparison
of two types using the Python interface, not check_types_equal.  The Python
bindings could change how testing equality between two types is implemented
such that it doesn't use check_types_equal anymore, and your test would still
test the Python interface's behavior.

Anyway, I find the separation in different directories very blurry.  If I write
a test case that uses two threads, should I automatically put it in gdb.threads
instead of gdb.base?

Simon