[RFA] ax-gdb: Do not treat enums and bools as integers.

[RFA] ax-gdb: Do not treat enums and bools as integers.

[Joel Brobecker]

This patch fixes a problem when using gdb + gdbserver, and trying
to break on a function when one of the (enum) parameters is equal
to a certain value, and the size of that enum is 1 byte.

    (gdb) break mixed.adb:15 if light = green
    Breakpoint 2 at 0x402d5a: file mixed.adb, line 15.
    (gdb) cont
    Continuing.
    [Inferior 1 (process 9742) exited normally]

The debugger should have stopped once when our function was call
with light set to green.

Here is what happens: Because we're using a recent GDBserver,
GDB hands off the evaluation of the condition to GDBserver, by
providing it in the Z0 packet. This is what GDB sends:

    $Z0,402d5a,1;X13,26000622100223ff1c16100219162022011327#cf

I decoded the condition as follow:

    260006    reg 6 -> push
    2210      const8 0x10 -> push
    02        add (stack now has 1 element equal to reg6 + 16)
    23ff1c    const16 0xff1c
    1610      ext 16 (sign extend 16 bits)
    02        add (stack now has 1 element equal to reg6 + 16 - 228)
    19        ref32: Pop as addr, push 32bit value at addr.
    1620      ext 32 (sign extend 32 bits)
    2201      const8 0x01
    13        equal
    27        end

The beginning of the agent expression can be explained by the address
of symbol "light":

    (gdb) info addr light
    Symbol "light" is a variable at frame base reg $rbp offset 16+-228.

However, the mistake is the "ext 32" operation (extend 32 bits),
because our variable is *not* 32bits, only 8:

    (gdb) print light'size
    $5 = 8

But the reason why GDB decides to use a 32bit extension is because
it overrides the symbol's type with a plain integer type in
ax-gdb.c:gen_usual_unary...

      /* If the value is an enum or a bool, call it an integer.  */
    case TYPE_CODE_ENUM:
    case TYPE_CODE_BOOL:
      value->type = builtin_type (exp->gdbarch)->builtin_int;
      break;

... before calling require_rvalue. And of course, that causes the
generator to generate a sizeof(int) extension of the result.

One way to fix this would be to use an integer type of the correct
size, but I do not understand why this is necessary. The two routines
that use that information to generate the opcode down the line are
gen_fetch (for a memory value), or gen_extend (for a register value).
And they both have handling of enums and bools.

So the fix we elected to implement was simply to remove that code.

gdb/ChangeLog:

        * ax-gdb.c (gen_usual_unary): Remove special handling of
        enum and bool types.

Tested on x86_64-linux.  I'll try to come up with an example that
reproduces the problem without using the proprietary testcase.

OK to commit?

---
 gdb/ax-gdb.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index 739677f..37882be 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -878,12 +878,6 @@ gen_usual_unary (struct expression *exp, struct agent_expr *ax,
     case TYPE_CODE_STRUCT:
     case TYPE_CODE_UNION:
       return;
-
-      /* If the value is an enum or a bool, call it an integer.  */
-    case TYPE_CODE_ENUM:
-    case TYPE_CODE_BOOL:
-      value->type = builtin_type (exp->gdbarch)->builtin_int;
-      break;
     }
 
   /* If the value is an lvalue, dereference it.  */
-- 
1.7.1

Re: [RFA] ax-gdb: Do not treat enums and bools as integers.

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * ax-gdb.c (gen_usual_unary): Remove special handling of
>         enum and bool types.
> 
> Tested on x86_64-linux.  I'll try to come up with an example that
> reproduces the problem without using the proprietary testcase.

Unfortunately, my attempts have failed. It comes from the fact
that somehow, the bytes right next to my parameter value are null
instead of being some random non-null value, thus hiding the problem.

-- 
Joel

Re: [RFA] ax-gdb: Do not treat enums and bools as integers.

[Tom Tromey]

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

Joel> But the reason why GDB decides to use a 32bit extension is because
Joel> it overrides the symbol's type with a plain integer type in
Joel> ax-gdb.c:gen_usual_unary...

FWIW you can see the bug from C, if you make an enum type and then
compile with -fshort-enums (which IIRC is the default for some targets).

Then:

    (gdb) maintenance agent-eval e
    Scope: 0x400478
    Reg mask: 00
      0  const32 6293576
      5  ref8
      6  end

Here the 'ref8' is correct -- 'e' is a 1-byte variable.

But:

    (gdb) maintenance agent-eval e + 1
    Scope: 0x400478
    Reg mask: 00
      0  const32 6293576
      5  ref32
      6  ext 32
      8  const8 1
     10  add
     11  ext 32
     13  end

Whoops, how did that ref32 get there?

Joel>         * ax-gdb.c (gen_usual_unary): Remove special handling of
Joel>         enum and bool types.

I think it is correct.


Also, I think you can make a test case like this:

    enum EE {
      VALUE = 1
    };

    struct x {
      unsigned char before;
      enum EE e;
      unsigned char after;
    };

    struct x global;

    int main () { } 

Compile with -fshort-enums.

Then:

    (gdb) maint agent-eval global.e + 1
    Scope: 0x400478
    Reg mask: 00
      0  const32 6293576
      5  const8 1
      7  add
      8  ref32
      9  ext 32
     11  const8 1
     13  add
     14  ext 32
     16  end

So, I think you could provoke the wrong answer by setting 'after' to
some non-zero value.

Tom

Re: [RFA] ax-gdb: Do not treat enums and bools as integers.

[Joel Brobecker]

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

> FWIW you can see the bug from C, if you make an enum type and then
> compile with -fshort-enums (which IIRC is the default for some targets).

Genius! I did not know this switch for C...

Attached is the testcase I created for it. Tested on x86_64-linux,
with and without gdbserver. In the gdbserver case, I verified that
it fails without the fix.

> Joel>         * ax-gdb.c (gen_usual_unary): Remove special handling of
> Joel>         enum and bool types.
> 
> I think it is correct.

Thanks for review. Patch and testcase now both checked in.

-- 
Joel

[-- Attachment #2: 0001-Testcase-for-ax-gdb-Do-not-treat-enums-and-bools-as-.patch --]
[-- Type: text/x-diff, Size: 4227 bytes --]

From 48e165d8e2f9f86ff8fc9271a4150793a6a17eae Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 13 Mar 2012 18:28:22 -0700
Subject: [PATCH] Testcase for: "ax-gdb: Do not treat enums and bools as integers".

gdb/testsuite/ChangeLog:

        * gdb.base/enum_cond.c, gdb.base/enum_cond.exp: New testcase.
---
 gdb/testsuite/ChangeLog              |    4 +++
 gdb/testsuite/gdb.base/enum_cond.c   |   48 ++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/enum_cond.exp |   44 +++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/enum_cond.c
 create mode 100644 gdb/testsuite/gdb.base/enum_cond.exp

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c05bcf4..f83a8ba 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,9 @@
 2012-03-13  Joel Brobecker  <brobecker@adacore.com>
 
+	* gdb.base/enum_cond.c, gdb.base/enum_cond.exp: New testcase.
+
+2012-03-13  Joel Brobecker  <brobecker@adacore.com>
+
 	* gdb.ada/bp_range_type: New testcase.
 
 2012-03-13  Doug Evans  <dje@google.com>
diff --git a/gdb/testsuite/gdb.base/enum_cond.c b/gdb/testsuite/gdb.base/enum_cond.c
new file mode 100644
index 0000000..5c152a4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/enum_cond.c
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+   Copyright 2012 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 EE
+{
+  VALUE = 1
+};
+
+struct x
+{
+  unsigned char before;
+  enum EE e;
+  unsigned char after;
+};
+
+
+int
+call_me (struct x param)
+{
+  return param.e;
+}
+
+int
+main (void)
+{
+  struct x val;
+
+  val.before = 0xff;
+  val.e = VALUE;
+  val.after = 0xff;
+
+  call_me (val);
+  return 0;
+}
+
diff --git a/gdb/testsuite/gdb.base/enum_cond.exp b/gdb/testsuite/gdb.base/enum_cond.exp
new file mode 100644
index 0000000..5c041d9
--- /dev/null
+++ b/gdb/testsuite/gdb.base/enum_cond.exp
@@ -0,0 +1,44 @@
+# Copyright 2012 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/>.
+
+# This file is part of the gdb testsuite.  It is intended to test that
+# gdb can correctly print arrays with indexes for each element of the
+# array.
+
+set testfile "enum_cond"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+set opts [list debug additional_flags=-fshort-enums]
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $opts] != "" } {
+    untested "Could not compile ${srcfile}"
+    return -1
+}
+
+clean_restart $testfile
+
+if ![runto_main] then {
+    perror "could not run to main"
+    continue
+}
+
+gdb_test "break call_me if param.e == 1" \
+         "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal\\."
+
+# Continue.  We should hit our breakpoint...
+gdb_test "continue" \
+         "Breakpoint $decimal, call_me \\(param=\\.\\.\\.\\) at .*" \
+         "continue to conditional breakpoint in call_me"
+
-- 
1.7.1