This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PING][RFC][PATCH v2] Python API: add gdb.stack_may_be_invalid
- From: Doug Evans <dje at google dot com>
- To: Martin Galvan <martin dot galvan at tallertechnologies dot com>
- Cc: Ulrich Weigand <uweigand at de dot ibm dot com>, gdb-patches <gdb-patches at sourceware dot org>, Eli Zaretskii <eliz at gnu dot org>, Pedro Alves <palves at redhat dot com>, Daniel Gutson <daniel dot gutson at tallertechnologies dot com>
- Date: Wed, 12 Nov 2014 09:06:23 -0800
- Subject: Re: [PING][RFC][PATCH v2] Python API: add gdb.stack_may_be_invalid
- Authentication-results: sourceware.org; auth=none
- References: <CAOKbPbZd+ppseGQW2OirBm4y5O=LgUMP-Pf8=RF00hnPOuMutw at mail dot gmail dot com> <201411071727 dot sA7HRNIQ007851 at d03av02 dot boulder dot ibm dot com> <CAOKbPbY24zgvHdmAYQvR6H=sCsF6ixzwvwdbhPpBkcdcsMotiw at mail dot gmail dot com> <CAOKbPbaa2jZonzn-tcH9C8ge5AVUJHJeREwWNLOokFqr7dd6vw at mail dot gmail dot com>
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}
> };