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

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

[Martin Galvan]

This is a continuation from this thread:

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

When single-stepping through machine instructions in non-optimized
code, sometimes the values of local variables may be invalid because
the stack frame is either not completely set up yet (in the function's
prologue) or destroyed (usually in the epilogue). This patch aims to
bring a way to determine whether this may happen at a given PC without
actually running the program.

In the previous thread we came to the conclusion that the notions of
"prologue" and "epilogue" don't exist in optimized code as they do in
unoptimized code. We shouldn't even have to worry about local
variables being unaccessible as the DWARF information is good enough.
However, the problem still remains in unoptimized code, so we'd still
need a solution for that case.

Initially, I used the now deprecated in_prologue function. Pedro Alves
suggested looking at how function breakpoints work instead (e.g.
"break myFunction" will place the breakpoint at the end of
myFunction's prologue). So I started reading the code and doing some
tests, and these are some of the things I saw:

1) As Gdb relies on the compiler placing the prologue on its own
"line" (that is, emmiting line info that advances only the address but
not the actual line), at many points in the code there's a check for
single-line functions, as people probably thought there may be a
problem with that behavior. That seems to be wrong, and was the cause
of some of my initial confusion. If you look at the DWARF info for one
such function, you'll see something like:

Special opcode 75: advance Address by 10 to 0x3b2 and Line by 0 to 38

Doing "break myFunction" will correctly place the breakpoint at 0x3b2.

2) The behavior of handle_step_into_function and setting breakpoints
is inconsistent for optimized code, at least in ARM. If you step into
a function in a program compiled with gcc -O1, you'll see the PC ends
up one instruction after the set of instructions that place the
arguments passed as registers in the registers they'll be used in. If
you do "break myFunction", however, the breakpoint will correctly be
placed at the very first instruction. Both handle_step.. and setting
breakpoints have the same effect on -O0 code.

3) This is the most important one: if you compare the DWARF info from
a -O0 binary to the one from a -O1 you'll notice the variables that
appear to be wrong in the prologue have a negative DW_OP_fbreg in
their DW_AT_location for -O0, and a location list for -O1. I believe
the presence of this location list is what makes the local variables
in -O1 always reliable. The locations_valid field in struct symtab has
the following comment (in symtab.h):

/* Symtab has been compiled with both optimizations and debug info so that
     GDB may stop skipping prologues as variables locations are valid already
     at function entry points.  */

The locations_valid field is checked in skip_prologue_sal, which is
called by find_function_start_sal if its "funfirstline" parameter is
non-zero. According to the comment in symtab.c:

"If the argument FUNFIRSTLINE is nonzero, we want the first line of
real code inside the function."

If we look at how "break myFunction" works, we'll see that we end up
calling find_function_start_sal to determine at which PC we have to
place our breakpoint. Therefore, that's the function we should be
calling when checking whether the stack frame will be valid at a
prologue, as it also accounts for optimizations.

As before, we take a conservative approach: we'll only be returning
False if we're certain that the local variables will be accessible at
the given PC. I changed the function names appropriately, and merged
both the prologue and epilogue functions into one. The start address
of the function is no longer a parameter, as we can get it from the
debug info itself.

I'm sending this patch as a RFC since I'd like to know what everyone
thinks of it before I bring in the documentation and test cases. As
before, thanks a lot for your feedback.

diff --git a/gdb/python/python.c b/gdb/python/python.c
index 41999d6..76c3f36 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -33,6 +33,7 @@
 #include "python.h"
 #include "extension-priv.h"
 #include "cli/cli-utils.h"
+#include "block.h"
 #include <ctype.h>

 /* Declared constants and enum for python stack printing.  */
@@ -703,6 +704,67 @@ 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
+   and -1 if we have no debug info to use. */
+
+static int
+pc_may_be_in_prologue (gdb_py_ulongest pc)
+{
+  struct symbol *function_symbol;
+  struct symtab_and_line function_body_start_sal;
+  int result = -1;
+
+  function_symbol = find_pc_function(pc);
+
+  if (function_symbol)
+    {
+      function_body_start_sal = find_function_start_sal (function_symbol, 1);
+
+      result = pc < function_body_start_sal.pc;
+    }
+
+    return result;
+}
+
+static int
+stack_is_destroyed (gdb_py_ulongest pc)
+{
+  return gdbarch_in_function_epilogue_p (python_gdbarch, pc);
+}
+
+/* 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 ?
+                   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."));
+        }
+    }
+
+  return result;
+}
+
 /* A Python function which is a wrapper for decode_line_1.  */

 static PyObject *
@@ -2000,6 +2062,13 @@ 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}
 };

-- 

Martín Galván

Software Engineer

Taller Technologies Argentina

San Lorenzo 47, 3rd Floor, Office 5

Córdoba, Argentina

Phone: 54 351 4217888 / +54 351 4218211

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

[Pedro Alves]

On 11/07/2014 01:32 PM, Martin Galvan wrote:
> 2) The behavior of handle_step_into_function and setting breakpoints
> is inconsistent for optimized code, at least in ARM. If you step into
> a function in a program compiled with gcc -O1, you'll see the PC ends
> up one instruction after the set of instructions that place the
> arguments passed as registers in the registers they'll be used in. If
> you do "break myFunction", however, the breakpoint will correctly be
> placed at the very first instruction. Both handle_step.. and setting
> breakpoints have the same effect on -O0 code.

We should really fix this.  I can't imagine we do this on purpose.

> If we look at how "break myFunction" works, we'll see that we end up
> calling find_function_start_sal to determine at which PC we have to
> place our breakpoint. Therefore, that's the function we should be
> calling when checking whether the stack frame will be valid at a
> prologue, as it also accounts for optimizations.

We expose functions and sals as python objects.
Shouldn't we instead consider exposing find_function_start_sal
in the function object?  Or maybe symbol_to_sal in the Symbol object?
I can well imagine these being useful to other use cases.

Thanks,
Pedro Alves

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

[Martin Galvan]

On Fri, Nov 7, 2014 at 2:09 PM, Pedro Alves <palves@redhat.com> wrote:
> On 11/07/2014 01:32 PM, Martin Galvan wrote:
>> 2) The behavior of handle_step_into_function and setting breakpoints
>> is inconsistent for optimized code, at least in ARM. If you step into
>> a function in a program compiled with gcc -O1, you'll see the PC ends
>> up one instruction after the set of instructions that place the
>> arguments passed as registers in the registers they'll be used in. If
>> you do "break myFunction", however, the breakpoint will correctly be
>> placed at the very first instruction. Both handle_step.. and setting
>> breakpoints have the same effect on -O0 code.
>
> We should really fix this.  I can't imagine we do this on purpose.

Indeed we should. I'll gladly fix this myself once we're done with this patch.

>> If we look at how "break myFunction" works, we'll see that we end up
>> calling find_function_start_sal to determine at which PC we have to
>> place our breakpoint. Therefore, that's the function we should be
>> calling when checking whether the stack frame will be valid at a
>> prologue, as it also accounts for optimizations.
>
> We expose functions and sals as python objects.
> Shouldn't we instead consider exposing find_function_start_sal
> in the function object?  Or maybe symbol_to_sal in the Symbol object?
> I can well imagine these being useful to other use cases.

We could do so, but for this use case we'd still need a way to check
if the stack has been destroyed by e.g. an epilogue.

If we want to make the API as primitive as possible, we could again
expose two functions instead of one (as in my previous patch).

-- 

Martín Galván

Software Engineer

Taller Technologies Argentina

San Lorenzo 47, 3rd Floor, Office 5

Córdoba, Argentina

Phone: 54 351 4217888 / +54 351 4218211

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

[Ulrich Weigand]

Martin Galvan wrote:

> +stack_is_destroyed (gdb_py_ulongest pc)
> +{
> +  return gdbarch_in_function_epilogue_p (python_gdbarch, pc);
> +}

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.

(python_gdbarch is unfortunately one of those hacks; it would be
preferable if we didn't have it at all ...)

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 that isn't possible, one fall-back might be to look up the
symbol table from the PC, and use the associated objfile arch.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[Martin Galvan]

On Fri, Nov 7, 2014 at 2:27 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Martin Galvan wrote:
>
>> +stack_is_destroyed (gdb_py_ulongest pc)
>> +{
>> +  return gdbarch_in_function_epilogue_p (python_gdbarch, pc);
>> +}
>
> 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.

Agreed. I used python_gdbarch because it's used all over the API, so I
thought I could use it in here.

> 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.

I'll look into this. Thanks a lot!

-- 

Martín Galván

Software Engineer

Taller Technologies Argentina

San Lorenzo 47, 3rd Floor, Office 5

Córdoba, Argentina

Phone: 54 351 4217888 / +54 351 4218211

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

[Martin Galvan]

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,
+   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)
+    {
+      function_body_start_sal = find_function_start_sal (function_symbol, 1);
+
+      result = pc < function_body_start_sal.pc;
+    }
+
+    return result;
+}
+
+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);
+    }
+
+  if (gdbarch != NULL)
+    {
+      result = gdbarch_in_function_epilogue_p (gdbarch, pc);
+    }
+  else
+    {
+      result = gdbarch_in_function_epilogue_p (python_gdbarch, pc);
+    }
+
+  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 ?
+                   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."));
+        }
+    }
+
+  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."},
+
+
   {NULL, NULL, 0, NULL}
 };

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

[Doug Evans]

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}
>  };

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

[Pedro Alves]

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

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

[Martin Galvan]

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!

-- 

Martín Galván

Software Engineer

Taller Technologies Argentina

San Lorenzo 47, 3rd Floor, Office 5

Córdoba, Argentina

Phone: 54 351 4217888 / +54 351 4218211

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

[Martin Galvan]

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.

-- 

Martín Galván

Software Engineer

Taller Technologies Argentina

San Lorenzo 47, 3rd Floor, Office 5

Córdoba, Argentina

Phone: 54 351 4217888 / +54 351 4218211

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

[Doug Evans]

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"