Re: [RFC] Python's gdb.FRAME_UNWIND_NULL_ID is no longer used. What to do with it?

[Pedro Alves <palves@redhat.com>]

On 12/02/2013 04:10 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I'd assume scripts just check the result of Frame.unwind_stop_reason,
> Pedro> and compare it to gdb.FRAME_UNWIND_NO_REASON.  That at most, they'll
> Pedro> pass the result of Frame.unwind_stop_reason to
> Pedro> gdb.frame_stop_reason_string.  I'd prefer to just get rid of it, but
> Pedro> it may be best to keep this around for compatibility, in case a script
> Pedro> does refer to gdb.FRAME_UNWIND_NULL_ID directly.
> 
> Pedro> In general, what's the policy for exposed constants like this in
> Pedro> Python?
> 
> My view is that we should provide compatibility with what we document.
> 
> That is, if it is in the docs, we should not remove it.
> 
> However, there is still some leeway for change.
> E.g., in this case, we don't document the value or type of the constant.
> This is intentional as it gives us some freedom to change the details --
> we don't generally want Python API promises to leak through to the C
> code.
> 
> Pedro> +/* This is no longer used anywhere, but it's kept because it's exposed
> Pedro> +   to Python.  This is how GDB used to indicate end of stack.  We've
> Pedro> +   now migrated to a model where frames always have a valid ID.  */
> Pedro>  SET (UNWIND_NULL_ID, "unwinder did not report frame ID")
>  
> For example you could remove this, and arrange to keep the Python
> constant around with some suitably chosen value.  I think it just has to
> be something that will never compare equal to any of the other
> constants, so just None would work.

I tried that, and then I remembered to try frame_stop_reason_string
on it:

(top-gdb) python print gdb.FRAME_UNWIND_NULL_ID
None
(top-gdb) python print gdb.FRAME_UNWIND_OUTERMOST
1
(top-gdb) python print gdb.frame_stop_reason_string(gdb.FRAME_UNWIND_OUTERMOST)
outermost
(top-gdb) python print gdb.frame_stop_reason_string(gdb.FRAME_UNWIND_NULL_ID)
Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: an integer is required
Error while executing Python code.

GDB itself won't ever generate FRAME_UNWIND_NULL_ID.  Do think
that's a problem? I guess we could set it to UNWIND_LAST+1
instead, and make gdbpy_frame_stop_reason_string handle it,
but at that point, I get to consider just leaving things
alone...

Hmm, or perhaps, we could implement this another way.  Leave
UNWIND_NULL_ID in unwind_stop_reasons.def, but define it
with a DEPRECATED_SET macro instead.  I guess that might
actually be better considering multiple extension languages
going forward, as then we have a single place to do this,
instead of having to repeat the "create deprecated constant"
exercise for all extension languages whenever something like
this happens.

--------------------
gdb.FRAME_UNWIND_NULL_ID is documented, so we need to keep it around.
However, we can just set it to None.

2013-12-12  Pedro Alves  <palves@redhat.com>
	    Tom Tromey  <tromey@redhat.com>

	* python/py-frame.c (gdbpy_initialize_frames): Add
	gdb.FRAME_UNWIND_NULL_ID as a deprecated constant.
	* python/py-utils.c (gdb_pymodule_add_deprecated_constant): New
	function.
	* python/python-internal.h (gdb_pymodule_add_deprecated_constant):
	Declare.
	* unwind_stop_reasons.def (UNWIND_NULL_ID): Delete.
---
 gdb/python/py-frame.c        |  6 ++++++
 gdb/python/py-utils.c        |  8 ++++++++
 gdb/python/python-internal.h | 14 ++++++++++++++
 gdb/unwind_stop_reasons.def  |  5 -----
 4 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 0f189f0..325b81b 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -649,6 +649,12 @@ gdbpy_initialize_frames (void)
 #include "unwind_stop_reasons.def"
 #undef SET

+  /* GDB core doesn't use this anymore, but since it was exposed to
+     Python before and documented, we can't remove it.  */
+  if (gdb_pymodule_add_deprecated_constant (gdb_module,
+					    "FRAME_UNWIND_NULL_ID") < 0)
+    return -1;
+
   return gdb_pymodule_addobject (gdb_module, "Frame",
 				 (PyObject *) &frame_object_type);
 }
diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
index 4dcd168..32340bf 100644
--- a/gdb/python/py-utils.c
+++ b/gdb/python/py-utils.c
@@ -442,3 +442,11 @@ gdb_pymodule_addobject (PyObject *module, const char *name, PyObject *object)
     Py_DECREF (object);
   return result;
 }
+
+/* See python-internal.h.  */
+
+int
+gdb_pymodule_add_deprecated_constant (PyObject *module, const char *name)
+{
+  return gdb_pymodule_addobject (gdb_module, name, Py_None);
+}
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 125670e..2fb0c8d 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -494,4 +494,18 @@ int gdb_pymodule_addobject (PyObject *module, const char *name,
 			    PyObject *object)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;

+/* Adds a deprecated constant to MODULE as NAME.  Return -1 on error,
+   0 on success.  The constant is set to None.
+
+   Rationale: We don't generally want Python API promises to leak
+   through to GDB's core C code.  We intentionaly usually don't
+   document the value or type of Python constants, as it gives us some
+   freedom to change the details.  When a core constant that was
+   exposed to Python is removed from the core, we arrange to keep the
+   Python constant around with a value that never compares equal to
+   any of the non-deprecated constants.  */
+int gdb_pymodule_add_deprecated_constant (PyObject *module,
+					  const char *name)
+  CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
+
 #endif /* GDB_PYTHON_INTERNAL_H */
diff --git a/gdb/unwind_stop_reasons.def b/gdb/unwind_stop_reasons.def
index 2c3d341..da67f21 100644
--- a/gdb/unwind_stop_reasons.def
+++ b/gdb/unwind_stop_reasons.def
@@ -31,11 +31,6 @@
    or we didn't fail.  */
 SET (UNWIND_NO_REASON, "no reason")

-/* This is no longer used anywhere, but it's kept because it's exposed
-   to Python.  This is how GDB used to indicate end of stack.  We've
-   now migrated to a model where frames always have a valid ID.  */
-SET (UNWIND_NULL_ID, "unwinder did not report frame ID")
-
 /* This frame is the outermost.  */
 SET (UNWIND_OUTERMOST, "outermost")

-- 
1.7.11.7