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

Martin Galvan
2014-11-12T17:24:35Z

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