Re: [RFC][Python] gdbpy_frame_stop_reason_string bug

[Kevin Pouget <kevin.pouget@gmail.com>]

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

On Fri, Oct 14, 2011 at 5:00 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Friday 14 October 2011 15:40:49, Kevin Pouget wrote:
>
>> 2011-10-14  Kevin Pouget  <kevin.pouget@st.com>
>>
>>       * frame.c (frame_stop_reason_string): Rewrite using
>>       unwind_stop_reasons.def.
>>       * frame.h (enum unwind_stop_reason): Likewise.
>>       * python/py-frame.c (gdbpy_initialize_frames): Likewise.
>>       (gdbpy_frame_stop_reason_string): Use new enum unwind_stop_reason
>>       constants for bound-checking.
>>       * unwind_stop_reasons.def: New file.
>>
>
> You're missing this change that was in my patch:
>
> Index: src/gdb/stack.c
> ===================================================================
> --- src.orig/gdb/stack.c        2011-10-11 12:43:20.000000000 +0100
> +++ src/gdb/stack.c     2011-10-12 15:38:23.083658404 +0100
> @@ -1625,7 +1625,7 @@ backtrace_command_1 (char *count_exp, in
>       enum unwind_stop_reason reason;
>
>       reason = get_frame_unwind_stop_reason (trailing);
> -      if (reason > UNWIND_FIRST_ERROR)
> +      if (reason >= UNWIND_FIRST_ERROR)
>        printf_filtered (_("Backtrace stopped: %s\n"),
>                         frame_stop_reason_string (reason));
>     }
>
> because before, UNWIND_FIRST_ERROR was not an alias, now it is.

fixed

> I notice only these two descriptions are capitalized:
>
>> +SET (UNWIND_NO_REASON, "No reason")
>
>> +SET (UNWIND_UNAVAILABLE, "Not enough registers or memory available to unwind further")
>
> (the latter my fault).  Can you lowercase them too for consistency?

fixed

> Also, I believe as is, we lose gdb.FRAME_UNWIND_FIRST_ERROR?

you're right, I've added it, as well as a description in the documentation:

> @item gdb.FRAME_UNWIND_FIRST_ERROR
> All the conditions after this alias are considered errors;
> abnormal stack termination. Current value is gdb.FRAME_UNWIND_UNAVAILABLE.

I'm not 100% convinced about the last part, but I feel like it would
be important to know at read time (instead of run time) what is
considered as an error and what isn't. Let me know if you want me to
remove/rephrase it.


Thanks,

Kevin

[-- Attachment #2: 0001-Move-unwind-reasons-to-an-external-.def-file.patch --]
[-- Type: text/x-patch, Size: 9626 bytes --]

From f37e4adb86342a694ae32b8a8737dc1860d85f53 Mon Sep 17 00:00:00 2001
From: Kevin Pouget <kevin.pouget@st.com>
Date: Mon, 17 Oct 2011 09:42:44 +0200
Subject: [PATCH] Move unwind reasons to an external .def file

---
 gdb/doc/gdb.texinfo         |    4 ++
 gdb/frame.c                 |   20 ++---------
 gdb/frame.h                 |   51 +++++----------------------
 gdb/python/py-frame.c       |   21 ++++-------
 gdb/stack.c                 |    2 +-
 gdb/unwind_stop_reasons.def |   79 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 106 insertions(+), 71 deletions(-)
 create mode 100644 gdb/unwind_stop_reasons.def

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 0aa90eb..e6e060c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -23445,6 +23445,10 @@ stack corruption.
 @item gdb.FRAME_UNWIND_NO_SAVED_PC
 The frame unwinder did not find any saved PC, but we needed
 one to unwind further.
+
+@item gdb.FRAME_UNWIND_FIRST_ERROR
+All the conditions after this alias are considered errors;
+abnormal stack termination. Current value is gdb.FRAME_UNWIND_UNAVAILABLE.
 @end table
 
 @end defun
diff --git a/gdb/frame.c b/gdb/frame.c
index 5824020..e5e442a 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2351,23 +2351,11 @@ frame_stop_reason_string (enum unwind_stop_reason reason)
 {
   switch (reason)
     {
-    case UNWIND_NULL_ID:
-      return _("unwinder did not report frame ID");
+#define SET(name, description) \
+    case name: return _(description);
+#include "unwind_stop_reasons.def"
+#undef SET
 
-    case UNWIND_UNAVAILABLE:
-      return _("Not enough registers or memory available to unwind further");
-
-    case UNWIND_INNER_ID:
-      return _("previous frame inner to this frame (corrupt stack?)");
-
-    case UNWIND_SAME_ID:
-      return _("previous frame identical to this frame (corrupt stack?)");
-
-    case UNWIND_NO_SAVED_PC:
-      return _("frame did not save the PC");
-
-    case UNWIND_NO_REASON:
-    case UNWIND_FIRST_ERROR:
     default:
       internal_error (__FILE__, __LINE__,
 		      "Invalid frame stop reason");
diff --git a/gdb/frame.h b/gdb/frame.h
index f5866bd..514393b 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -446,47 +446,16 @@ extern struct address_space *get_frame_address_space (struct frame_info *);
 
 enum unwind_stop_reason
   {
-    /* No particular reason; either we haven't tried unwinding yet,
-       or we didn't fail.  */
-    UNWIND_NO_REASON,
-
-    /* The previous frame's analyzer returns an invalid result
-       from this_id.
-
-       FIXME drow/2006-08-16: This is how GDB used to indicate end of
-       stack.  We should migrate to a model where frames always have a
-       valid ID, and this becomes not just an error but an internal
-       error.  But that's a project for another day.  */
-    UNWIND_NULL_ID,
-
-    /* This frame is the outermost.  */
-    UNWIND_OUTERMOST,
-
-    /* All the conditions after this point are considered errors;
-       abnormal stack termination.  If a backtrace stops for one
-       of these reasons, we'll let the user know.  This marker
-       is not a valid stop reason.  */
-    UNWIND_FIRST_ERROR,
-
-    /* Can't unwind further, because that would require knowing the
-       values of registers or memory that haven't been collected.  */
-    UNWIND_UNAVAILABLE,
-
-    /* This frame ID looks like it ought to belong to a NEXT frame,
-       but we got it for a PREV frame.  Normally, this is a sign of
-       unwinder failure.  It could also indicate stack corruption.  */
-    UNWIND_INNER_ID,
-
-    /* This frame has the same ID as the previous one.  That means
-       that unwinding further would almost certainly give us another
-       frame with exactly the same ID, so break the chain.  Normally,
-       this is a sign of unwinder failure.  It could also indicate
-       stack corruption.  */
-    UNWIND_SAME_ID,
-
-    /* The frame unwinder didn't find any saved PC, but we needed
-       one to unwind further.  */
-    UNWIND_NO_SAVED_PC,
+#define SET(name, description) name,
+#define FIRST_ENTRY(name) UNWIND_FIRST = name,
+#define LAST_ENTRY(name) UNWIND_LAST = name,
+#define FIRST_ERROR(name) UNWIND_FIRST_ERROR = name,
+
+#include "unwind_stop_reasons.def"
+#undef SET
+#undef FIRST_ENTRY
+#undef LAST_ENTRY
+#undef FIRST_ERROR
   };
 
 /* Return the reason why we can't unwind past this frame.  */
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 75aa44e..4983c82 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -542,7 +542,7 @@ gdbpy_frame_stop_reason_string (PyObject *self, PyObject *args)
   if (!PyArg_ParseTuple (args, "i", &reason))
     return NULL;
 
-  if (reason < 0 || reason > UNWIND_NO_SAVED_PC)
+  if (reason < UNWIND_FIRST || reason > UNWIND_LAST)
     {
       PyErr_SetString (PyExc_ValueError, 
 		       _("Invalid frame stop reason."));
@@ -599,18 +599,13 @@ gdbpy_initialize_frames (void)
   PyModule_AddIntConstant (gdb_module, "SIGTRAMP_FRAME", SIGTRAMP_FRAME);
   PyModule_AddIntConstant (gdb_module, "ARCH_FRAME", ARCH_FRAME);
   PyModule_AddIntConstant (gdb_module, "SENTINEL_FRAME", SENTINEL_FRAME);
-  PyModule_AddIntConstant (gdb_module,
-			   "FRAME_UNWIND_NO_REASON", UNWIND_NO_REASON);
-  PyModule_AddIntConstant (gdb_module,
-			   "FRAME_UNWIND_NULL_ID", UNWIND_NULL_ID);
-  PyModule_AddIntConstant (gdb_module,
-			   "FRAME_UNWIND_FIRST_ERROR", UNWIND_FIRST_ERROR);
-  PyModule_AddIntConstant (gdb_module,
-			   "FRAME_UNWIND_INNER_ID", UNWIND_INNER_ID);
-  PyModule_AddIntConstant (gdb_module,
-			   "FRAME_UNWIND_SAME_ID", UNWIND_SAME_ID);
-  PyModule_AddIntConstant (gdb_module,
-			   "FRAME_UNWIND_NO_SAVED_PC", UNWIND_NO_SAVED_PC);
+
+#define SET(name, description) \
+  PyModule_AddIntConstant (gdb_module, "FRAME_"#name, name);
+#define FIRST_ERROR(name) \
+  PyModule_AddIntConstant (gdb_module, "FRAME_"#name, name);
+#include "../unwind_stop_reasons.def"
+#undef SET
 
   Py_INCREF (&frame_object_type);
   PyModule_AddObject (gdb_module, "Frame", (PyObject *) &frame_object_type);
diff --git a/gdb/stack.c b/gdb/stack.c
index 953d3bd..003725a 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1625,7 +1625,7 @@ backtrace_command_1 (char *count_exp, int show_locals, int from_tty)
       enum unwind_stop_reason reason;
 
       reason = get_frame_unwind_stop_reason (trailing);
-      if (reason > UNWIND_FIRST_ERROR)
+      if (reason >= UNWIND_FIRST_ERROR)
 	printf_filtered (_("Backtrace stopped: %s\n"),
 			 frame_stop_reason_string (reason));
     }
diff --git a/gdb/unwind_stop_reasons.def b/gdb/unwind_stop_reasons.def
new file mode 100644
index 0000000..3eca2b2
--- /dev/null
+++ b/gdb/unwind_stop_reasons.def
@@ -0,0 +1,79 @@
+/* Copyright 2011 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+/* Reasons why frames could not be further unwound
+   SET (name, description)
+  
+   After this reason name, all reasons should be considered errors;
+   i.e.: abnormal stack termination.
+   FIRST_ERROR (name)  
+   
+   First and Last reason defined
+   FIRST_ENTRY (name)
+   LAST_ENTRY (name)  */
+
+#ifdef SET
+/* No particular reason; either we haven't tried unwinding yet, 
+   or we didn't fail.  */
+SET (UNWIND_NO_REASON, "no reason")
+
+/* The previous frame's analyzer returns an invalid result
+   from this_id.
+
+   FIXME drow/2006-08-16: This is how GDB used to indicate end of
+   stack.  We should migrate to a model where frames always have a
+   valid ID, and this becomes not just an error but an internal
+   error.  But that's a project for another day.  */
+SET (UNWIND_NULL_ID, "unwinder did not report frame ID")
+
+/* This frame is the outermost.  */
+SET (UNWIND_OUTERMOST, "outermost")
+
+/* Can't unwind further, because that would require knowing the
+   values of registers or memory that haven't been collected.  */
+SET (UNWIND_UNAVAILABLE, "not enough registers or memory available to unwind further")
+
+/* This frame ID looks like it ought to belong to a NEXT frame,
+   but we got it for a PREV frame.  Normally, this is a sign of
+   unwinder failure.  It could also indicate stack corruption.  */
+SET (UNWIND_INNER_ID, "previous frame inner to this frame (corrupt stack?)")
+
+/* This frame has the same ID as the previous one.  That means
+   that unwinding further would almost certainly give us another
+   frame with exactly the same ID, so break the chain.  Normally,
+   this is a sign of unwinder failure.  It could also indicate
+   stack corruption.  */
+SET (UNWIND_SAME_ID, "previous frame identical to this frame (corrupt stack?)")
+
+/* The frame unwinder didn't find any saved PC, but we needed
+   one to unwind further.  */
+SET (UNWIND_NO_SAVED_PC, "frame did not save the PC")
+
+#endif /* SET */
+
+
+#ifdef FIRST_ERROR
+FIRST_ERROR (UNWIND_UNAVAILABLE)
+#endif
+
+#ifdef FIRST_ENTRY
+FIRST_ENTRY (UNWIND_NO_REASON)
+#endif
+
+#ifdef LAST_ENTRY
+LAST_ENTRY (UNWIND_NO_SAVED_PC)
+#endif
-- 
1.7.6.4