[PING,RFC,v2] Python API: add gdb.stack_may_be_invalid

  On Fri, Nov 7, 2014 at 2:36 PM, Martin Galvan
<martin.galvan@tallertechnologies.com> wrote:
> On Fri, Nov 7, 2014 at 2:27 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> Just one comment here: python_gdbarch isn't really correct here.
>> If you have a platform that supports multiple architectures, then
>> you really should use the appropriate gdbarch for PC.
>>
>> Ideally, the Python interface should carry enough information to
>> determine the appropriate gdbarch, e.g. by operating on a Frame
>> instead of a plain PC value.
>
> If I understand correctly, using a Frame would require the program to
> be already running by the time we call the API function, which isn't
> really what we want.
>
>> If that isn't possible, one fall-back might be to look up the
>> symbol table from the PC, and use the associated objfile arch.

Here's the new version of the patch. It uses the objfile's gdbarch
and, if not available, python_gdbarch.

 static PyObject *
@@ -2000,6 +2081,15 @@ Return the selected inferior object." },
   { "inferiors", gdbpy_inferiors, METH_NOARGS,
     "inferiors () -> (gdb.Inferior, ...).\n\
 Return a tuple containing all inferiors." },
+
+
+  { "stack_may_be_invalid", gdbpy_stack_may_be_invalid, METH_VARARGS,
+    "stack_may_be_invalid (Long) -> Boolean.\n\
+Returns True if a given PC may point to an address in which the stack frame\n\
+may not be valid (either because it may not be set up yet or because it was\n\
+destroyed, usually in a function's epilogue), False otherwise."},
+
+
   {NULL, NULL, 0, NULL}
 };
  

On Wed, Nov 12, 2014 at 7:55 AM, Martin Galvan
<martin.galvan@tallertechnologies.com> wrote:
> On Fri, Nov 7, 2014 at 2:36 PM, Martin Galvan
> <martin.galvan@tallertechnologies.com> wrote:
>> On Fri, Nov 7, 2014 at 2:27 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>>> Just one comment here: python_gdbarch isn't really correct here.
>>> If you have a platform that supports multiple architectures, then
>>> you really should use the appropriate gdbarch for PC.
>>>
>>> Ideally, the Python interface should carry enough information to
>>> determine the appropriate gdbarch, e.g. by operating on a Frame
>>> instead of a plain PC value.
>>
>> If I understand correctly, using a Frame would require the program to
>> be already running by the time we call the API function, which isn't
>> really what we want.
>>
>>> If that isn't possible, one fall-back might be to look up the
>>> symbol table from the PC, and use the associated objfile arch.
>
> Here's the new version of the patch. It uses the objfile's gdbarch
> and, if not available, python_gdbarch.
>
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index d23325a..2dc2d41 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -703,6 +703,87 @@ gdbpy_solib_name (PyObject *self, PyObject *args)
>    return str_obj;
>  }
>
> +/* Returns 1 if the given PC may be inside a prologue, 0 if it
> definitely isn't,

Hi.  A few comments.

Broken patch.  Cut-n-paste error or unhelpful mail program?

> +   and -1 if we have no debug info to use. */
> +
> +static int
> +pc_may_be_in_prologue (gdb_py_ulongest pc)
> +{
> +  int result = -1;
> +  struct symbol *function_symbol;
> +  struct symtab_and_line function_body_start_sal;
> +
> +  function_symbol = find_pc_function(pc);
> +
> +  if (function_symbol)

gdb's coding style convention is to write function_symbol != NULL.

> +    {
> +      function_body_start_sal = find_function_start_sal (function_symbol, 1);
> +
> +      result = pc < function_body_start_sal.pc;

IWBN if the higher level API provided a routine rather than the python
code having to hand-code this test.  IOW, "pc_may_be_in_prologue"
should live in gdb/*.c, not gdb/python/*.c.
[As for which file, in gdb/*.c, symtab.c would be fine for now I think.]

> +    }
> +
> +    return result;
> +}
> +

Missing function comment for stack_is_destroyed.
As a rule they all must have them.

Plus the name "stack is destroyed" is confusing.
This function is just a wrapper around gdbarch_in_function_epilogue_p
so I'd just call it in_function_epilogue_p (or
gdbpy_in_function_epilogue_p or some such).

> +static int
> +stack_is_destroyed (gdb_py_ulongest pc)
> +{
> +  int result;
> +  struct symtab *symtab = NULL;
> +  struct gdbarch *gdbarch = NULL;
> +
> +  symtab = find_pc_symtab (pc);
> +
> +  if ((symtab != NULL) && (symtab->objfile != NULL))
> +    {
> +      gdbarch = get_objfile_arch (symtab->objfile);
> +    }

Convention is to not use braces when the code occupies one line.

> +
> +  if (gdbarch != NULL)
> +    {
> +      result = gdbarch_in_function_epilogue_p (gdbarch, pc);
> +    }
> +  else
> +    {
> +      result = gdbarch_in_function_epilogue_p (python_gdbarch, pc);
> +    }

This code would be simpler if written as:

   if (gdbarch == NULL)
     gdbarch = python_gdbarch;

  result = gdbarch_function_in_epilogue_p (python_gdbarch);

> +
> +  return result;
> +}
> +
> +/* Returns True if a given PC may point to an address in which the stack frame
> +   may not be valid (either because it may not be set up yet or because it was
> +   destroyed, usually in a function's epilogue), False otherwise. */
> +
> +static PyObject *
> +gdbpy_stack_may_be_invalid (PyObject *self, PyObject *args)
> +{
> +  gdb_py_ulongest pc;
> +  PyObject *result = NULL;
> +  int pc_maybe_in_prologue;
> +
> +  if (PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &pc))
> +    {
> +      pc_maybe_in_prologue = pc_may_be_in_prologue (pc);
> +
> +      if (pc_maybe_in_prologue != -1)
> +        {
> +          result = stack_is_destroyed (pc) || pc_maybe_in_prologue ?

It'd be more efficient to avoid an unnecessary call to
stack_is_destroyed by checking pc_maybe_in_prologue first.

> +                   Py_True : Py_False;
> +
> +          Py_INCREF (result);
> +        }
> +      else  /* No debug info at that point. */
> +        {
> +          PyErr_Format (PyExc_RuntimeError,
> +                        _("There's no debug info for a function that\n"
> +                          "could be enclosing the given PC."));

A newline in an error message feels odd.
I'd remove it.

> +        }
> +    }
> +
> +  return result;
> +}
> +
>  /* A Python function which is a wrapper for decode_line_1.  */
>
>  static PyObject *
> @@ -2000,6 +2081,15 @@ Return the selected inferior object." },
>    { "inferiors", gdbpy_inferiors, METH_NOARGS,
>      "inferiors () -> (gdb.Inferior, ...).\n\
>  Return a tuple containing all inferiors." },
> +
> +
> +  { "stack_may_be_invalid", gdbpy_stack_may_be_invalid, METH_VARARGS,
> +    "stack_may_be_invalid (Long) -> Boolean.\n\
> +Returns True if a given PC may point to an address in which the stack frame\n\
> +may not be valid (either because it may not be set up yet or because it was\n\
> +destroyed, usually in a function's epilogue), False otherwise."},

The name "stack_may_be_invalid" is confusing.
It's not that the stack is invalid, rather that locals in the stack
frame are inaccessible.
stack_frame_may_be_invalid?

> +
> +
>    {NULL, NULL, 0, NULL}
>  };
  

On 11/12/2014 05:06 PM, Doug Evans wrote:
> 
> Plus the name "stack is destroyed" is confusing.
> This function is just a wrapper around gdbarch_in_function_epilogue_p
> so I'd just call it in_function_epilogue_p (or
> gdbpy_in_function_epilogue_p or some such).

Can we agree to rename the gdbarch hook instead?
The function does _not_ return whether the PC is in the epilogue.

https://sourceware.org/ml/gdb-patches/2014-10/msg00590.html

Thanks,
Pedro Alves
  

On Wed, Nov 12, 2014 at 2:06 PM, Doug Evans <dje@google.com> wrote:
> Broken patch.  Cut-n-paste error or unhelpful mail program?

Probably the second one. Will fix it for the next version.

>> +   and -1 if we have no debug info to use. */
>> +
>> +static int
>> +pc_may_be_in_prologue (gdb_py_ulongest pc)
>> +{
>> +  int result = -1;
>> +  struct symbol *function_symbol;
>> +  struct symtab_and_line function_body_start_sal;
>> +
>> +  function_symbol = find_pc_function(pc);
>> +
>> +  if (function_symbol)
>
> gdb's coding style convention is to write function_symbol != NULL.

Indeed, I must've missed that one!

>> +    {
>> +      function_body_start_sal = find_function_start_sal (function_symbol, 1);
>> +
>> +      result = pc < function_body_start_sal.pc;
>
> IWBN if the higher level API provided a routine rather than the python
> code having to hand-code this test.  IOW, "pc_may_be_in_prologue"
> should live in gdb/*.c, not gdb/python/*.c.
> [As for which file, in gdb/*.c, symtab.c would be fine for now I think.]
>> +    }
>> +
>> +    return result;
>> +}
>> +
>
> Missing function comment for stack_is_destroyed.
> As a rule they all must have them.

Will do.

> Plus the name "stack is destroyed" is confusing.
> This function is just a wrapper around gdbarch_in_function_epilogue_p
> so I'd just call it in_function_epilogue_p (or
> gdbpy_in_function_epilogue_p or some such).

In the last thread (
https://www.sourceware.org/ml/gdb-patches/2014-10/msg00590.html ) we
discussed that and Pedro pointed out that
gdbarch_in_function_epilogue_p itself is misnamed, and we shouldn't
carry that confusion to the Python API.

>> +static int
>> +stack_is_destroyed (gdb_py_ulongest pc)
>> +{
>> +  int result;
>> +  struct symtab *symtab = NULL;
>> +  struct gdbarch *gdbarch = NULL;
>> +
>> +  symtab = find_pc_symtab (pc);
>> +
>> +  if ((symtab != NULL) && (symtab->objfile != NULL))
>> +    {
>> +      gdbarch = get_objfile_arch (symtab->objfile);
>> +    }
>
> Convention is to not use braces when the code occupies one line.

As you wish.

>> +
>> +  if (gdbarch != NULL)
>> +    {
>> +      result = gdbarch_in_function_epilogue_p (gdbarch, pc);
>> +    }
>> +  else
>> +    {
>> +      result = gdbarch_in_function_epilogue_p (python_gdbarch, pc);
>> +    }
>
> This code would be simpler if written as:
>
>    if (gdbarch == NULL)
>      gdbarch = python_gdbarch;
>
>   result = gdbarch_function_in_epilogue_p (python_gdbarch);
>> +
>> +  return result;
>> +}
>> +
>> +/* Returns True if a given PC may point to an address in which the stack frame
>> +   may not be valid (either because it may not be set up yet or because it was
>> +   destroyed, usually in a function's epilogue), False otherwise. */
>> +
>> +static PyObject *
>> +gdbpy_stack_may_be_invalid (PyObject *self, PyObject *args)
>> +{
>> +  gdb_py_ulongest pc;
>> +  PyObject *result = NULL;
>> +  int pc_maybe_in_prologue;
>> +
>> +  if (PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &pc))
>> +    {
>> +      pc_maybe_in_prologue = pc_may_be_in_prologue (pc);
>> +
>> +      if (pc_maybe_in_prologue != -1)
>> +        {
>> +          result = stack_is_destroyed (pc) || pc_maybe_in_prologue ?
>
> It'd be more efficient to avoid an unnecessary call to
> stack_is_destroyed by checking pc_maybe_in_prologue first.
>> +                   Py_True : Py_False;
>> +
>> +          Py_INCREF (result);
>> +        }
>> +      else  /* No debug info at that point. */
>> +        {
>> +          PyErr_Format (PyExc_RuntimeError,
>> +                        _("There's no debug info for a function that\n"
>> +                          "could be enclosing the given PC."));
>
> A newline in an error message feels odd.
> I'd remove it.
>> +        }
>> +    }
>> +
>> +  return result;
>> +}
>> +
>>  /* A Python function which is a wrapper for decode_line_1.  */
>>
>>  static PyObject *
>> @@ -2000,6 +2081,15 @@ Return the selected inferior object." },
>>    { "inferiors", gdbpy_inferiors, METH_NOARGS,
>>      "inferiors () -> (gdb.Inferior, ...).\n\
>>  Return a tuple containing all inferiors." },
>> +
>> +
>> +  { "stack_may_be_invalid", gdbpy_stack_may_be_invalid, METH_VARARGS,
>> +    "stack_may_be_invalid (Long) -> Boolean.\n\
>> +Returns True if a given PC may point to an address in which the stack frame\n\
>> +may not be valid (either because it may not be set up yet or because it was\n\
>> +destroyed, usually in a function's epilogue), False otherwise."},
>
> The name "stack_may_be_invalid" is confusing.
> It's not that the stack is invalid, rather that locals in the stack
> frame are inaccessible.
> stack_frame_may_be_invalid?

Indeed, will fix those as well. Thanks a lot for the feedback!
  

On Wed, Nov 12, 2014 at 2:20 PM, Pedro Alves <palves@redhat.com> wrote:
> On 11/12/2014 05:06 PM, Doug Evans wrote:
>>
>> Plus the name "stack is destroyed" is confusing.
>> This function is just a wrapper around gdbarch_in_function_epilogue_p
>> so I'd just call it in_function_epilogue_p (or
>> gdbpy_in_function_epilogue_p or some such).
>
> Can we agree to rename the gdbarch hook instead?
> The function does _not_ return whether the PC is in the epilogue.
>
> https://sourceware.org/ml/gdb-patches/2014-10/msg00590.html

If everyone's ok with stack_frame_destroyed_p, I'll do the renaming
myself and send it as a separate patch.
  

On Wed, Nov 12, 2014 at 9:20 AM, Pedro Alves <palves@redhat.com> wrote:
> On 11/12/2014 05:06 PM, Doug Evans wrote:
>>
>> Plus the name "stack is destroyed" is confusing.
>> This function is just a wrapper around gdbarch_in_function_epilogue_p
>> so I'd just call it in_function_epilogue_p (or
>> gdbpy_in_function_epilogue_p or some such).
>
> Can we agree to rename the gdbarch hook instead?
> The function does _not_ return whether the PC is in the epilogue.
>
> https://sourceware.org/ml/gdb-patches/2014-10/msg00590.html

"works for me"