Re: [PATCH] Python API: Add gdb.is_in_prologue and gdb.is_in_epilogue.

[Pedro Alves <palves at redhat dot com>]

On 10/22/2014 03:01 PM, Martin Galvan wrote:
> Added two new functions to the Python API: gdb.is_in_prologue and gdb.is_in_epilogue.
> These expose the in_prologue and gdbarch_in_function_epilogue_p functions
> respectively, which are useful for checking if the values of local variables are
> valid when single-stepping through machine instructions.

gdbarch_in_function_epilogue_p is misnamed, see:

 https://sourceware.org/ml/gdb-patches/2014-08/msg00592.html

>From that thread:

> > # A target might have problems with watchpoints as soon as the stack
> > # frame of the current function has been destroyed.  This mostly happens
> > # as the first action in a funtion's epilogue.  in_function_epilogue_p()
> > # is defined to return a non-zero value if either the given addr is one
> >                                                                  ^^^^^^
> > # instruction after the stack destroying instruction up to the trailing
> >   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > # return instruction or if we can figure out that the stack frame has
> > # already been invalidated regardless of the value of addr.  Targets
> > # which don't suffer from that problem could just let this functionality
> > # untouched.
> > m:int:in_function_epilogue_p:CORE_ADDR addr:addr:0:generic_in_function_epilogue_p::0
> >
>
> Yes, this gdbarch hook's name is misleading.  How about renaming it to
> stack_frame_destroyed_p?

(I agreed to that name.)

target's whose gdbarch_in_function_epilogue_p is returning true for
instructions in the epilogue before the stack is destroyed, will
have software watchpoints broken in the tail call case Yao described,
like ARM had.

And so continuing with the in_function_epilogue_p name results in a
misnamed Python hook too; I'd much rather not carry that confusion to
the exported API.  That function could well return true if for some
reason we're stopped at code in the middle of the function, not
an in epilogue, that for some reason destroys the stack.

Or, consider the case of at some point we wanting to expose a
function that actually returns whether the PC is in the
epilogue, no matter whether the stack/frame has been destroyed
or not.

> 
> Also added tests for gdb.is_in_prologue only. The reason of this is that
> the tests work by checking the first instruction of a given function, as that's
> conventionally part of the prologue. Testing gdb.is_in_epilogue seems to be
> architecture-dependant (for instance, the last instruction of a function is
> reported as part of an epilogue by gdbarch_in_function_epilogue_p for ARM,
> but not always for x86_64), so I didn't include a test for it.

See above.  This confusion is a consequence of the bad function name.

How about a test that single-steps out of a function, and checks
in_function_epilogue at each single-step until the function is
finished?  We can use istarget "foo" to tweak whether to expect
whether in_function_epilogue returns true in one of the steps
or not.

> 
> gdb/ChangeLog
> 
> 2014-10-22  Martin Galvan  <martin.galvan@tallertechnologies.com>
> 
>     * NEWS: Mention new Python functions.
>     * python/python.c:
>     (gdbpy_is_in_prologue)
>     (gdbpy_is_in_epilogue): New functions.
> 
> gdb/doc/ChangeLog
> 
> 2014-10-22  Martin Galvan  <martin.galvan@tallertechnologies.com>
> 
>     * python.texi (Basic Python): Document new functions gdb.is_in_prologue and
>     gdb.is_in_epilogue.
> 
> gdb/testsuite/ChangeLog
> 
> 2014-10-22  Martin Galvan  <martin.galvan@tallertechnologies.com>
> 
>     * gdb.python/python.exp: Add tests for gdb.is_in_prologue.
> ---
>  gdb/NEWS                            |  3 ++
>  gdb/doc/python.texi                 | 45 +++++++++++++++++++++++++++
>  gdb/python/python.c                 | 61 +++++++++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.python/python.exp | 44 ++++++++++++++++++++++++++
>  4 files changed, 153 insertions(+)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 606fd16..31959bb 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -13,6 +13,9 @@
>       which is the gdb.Progspace object of the containing program space.
>    ** A new event "gdb.clear_objfiles" has been added, triggered when
>       selecting a new file to debug.
> +  ** Two new functions: gdb.is_in_prologue and gdb.is_in_epilogue,
> +     which are wrappers for in_prologue and gdbarch_in_function_epilogue_p
> +     respectively.

This is talking about wrapping some GDB internals.  I think it'd be
better to say something in user-speak.  The implementations of
these new Python functions could change, even.


>  
>  * New Python-based convenience functions:
>  
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index f1fd841..d87913a 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -440,6 +440,51 @@ such as those used by readline for command input, and annotation
>  related prompts are prohibited from being changed.
>  @end defun
>  
> +@findex gdb.is_in_prologue
> +@defun gdb.is_in_prologue (pc, @r{[}functionStart@r{]})
> +Returns @code{True} if the given @var{pc} value *might* correspond to
> +an instruction that executes before the stack frame of its containing
> +function is completely set up, @code{False} if it definitely doesn't.
> +
> +When stepping by machine instructions it's possible that local variables
> +appear to have wrong values at the beginning of a function.  This
> +happens because it usually takes more than one instruction to set up
> +a stack frame (including local variable definitions); such instructions
> +are part of a function's prologue.  @value{GDBN} can identify the
> +addresses where the local variables may show wrong values and inform
> +you so.  @value{GDBN} will usually take a "conservative" approach when
> +analyzing the prologue, assuming the result will be @code{True} unless
> +it's completely sure it won't.  As such, sometimes a given @var{pc} may
> +be reported as being before the stack frame is set up when it actually
> +isn't; however if the result is @code{False} you can be sure
> +@value{GDBN} is right.
> +
> +The optional @var{functionStart} argument is the start address of the
> +function you want to check if @var{pc} belongs to.  If your binary
> +doesn't have debugging info, @value{GDBN} may need to use this value
> +to guess if @var{pc} belongs to the prologue.  If omitted it defaults
> +to 0.

Some targets have code at address 0.  Seems like we may be exposing a
bad interface for these targets here?

> +
> +In general you shouldn't worry about passing a @var{functionStart}
> +argument unless your binary doesn't have debugging info, in which case
> +ommiting @var{functionStart} may result in @code{True} being returned
> +when the @var{pc} is not actually inside a prologue.
> +@end defun
> +
> +@findex gdb.is_in_epilogue
> +@defun gdb.is_in_epilogue (pc)
> +Returns @code{True} if the given @var{pc} value corresponds to an
> +instruction that executes after the stack of its containing function
> +has been destroyed, @code{False} if it doesn't.
> +
> +When stepping by machine instructions it's possible that local variables
> +appear to have wrong values at the end of a function.  This happens
> +because it usually takes more than one instruction to tear down a stack
> +frame; such instructions are part of a function's epilogue.  @value{GDBN}
> +can identify the addresses where the local variables may show wrong
> +values and inform you so.
> +@end defun
> +
>  @node Exception Handling
>  @subsubsection Exception Handling
>  @cindex python exceptions
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index ca531e2..b1b4422 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -703,6 +703,55 @@ gdbpy_solib_name (PyObject *self, PyObject *args)
>    return str_obj;
>  }
>  
> +/* A Python wrapper for in_prologue. */
> +
> +static PyObject *
> +gdbpy_is_in_prologue (PyObject *self, PyObject *args, PyObject *kw)
> +{
> +  /* If the user doesn't provide a function start address, assume 0 and hope
> +     we have at least minimal symbols. If we don't, we might be returning
> +     a false positive */
> +  gdb_py_longest pc;
> +  gdb_py_longest function_start = 0;
> +  const PyObject *result;
> +  char *keywords[] = {"pc", "functionStart", NULL};
> +
> +  if (PyArg_ParseTupleAndKeywords (args, kw,
> +  				   GDB_PY_LLU_ARG "|" GDB_PY_LLU_ARG,
> +  				   keywords, &pc, &function_start))
> +    {
> +      result = in_prologue (python_gdbarch, pc, function_start) ? Py_True : Py_False;
> +      Py_INCREF (result);
> +    }
> +  else /* Couldn't parse the given args. */
> +    {
> +      result = NULL;
> +    }
> +
> +  return result;
> +}
> +
> +/* A Python wrapper for gdbarch_in_function_epilogue_p. */
> +
> +static PyObject *
> +gdbpy_is_in_epilogue (PyObject *self, PyObject *args)
> +{
> +  gdb_py_longest pc;
> +  const PyObject* result;
> +
> +  if (PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &pc))
> +    {
> +      result = gdbarch_in_function_epilogue_p (python_gdbarch, pc) ? Py_True : Py_False;
> +      Py_INCREF (result);
> +    }
> +  else /* Couldn't parse the given args. */
> +    {
> +      result = NULL;
> +    }
> +
> +  return result;
> +}
> +
>  /* A Python function which is a wrapper for decode_line_1.  */
>  
>  static PyObject *
> @@ -2000,6 +2049,18 @@ Return the selected inferior object." },
>    { "inferiors", gdbpy_inferiors, METH_NOARGS,
>      "inferiors () -> (gdb.Inferior, ...).\n\
>  Return a tuple containing all inferiors." },
> +
> +  { "is_in_prologue", gdbpy_is_in_prologue, METH_VARARGS | METH_KEYWORDS,
> +    "is_in_prologue (pc, functionStart) -> Boolean.\n\
> +Returns True if the given pc value *might* correspond to an instruction\n\
> +that executes before the stack of its containing function is completely set up,\n\
> +False if it definitely doesn't."},
> +  { "is_in_epilogue", gdbpy_is_in_epilogue, METH_VARARGS | METH_KEYWORDS,
> +    "is_in_epilogue (pc) -> Boolean.\n\
> +Returns True if the given pc value corresponds to an instruction\n\
> +that executes after the stack of its containing function has been destroyed,\n\
> +False if it doesn't."},
> +
>    {NULL, NULL, 0, NULL}
>  };
>  
> diff --git a/gdb/testsuite/gdb.python/python.exp b/gdb/testsuite/gdb.python/python.exp
> index 3df9347..1611e0b 100644
> --- a/gdb/testsuite/gdb.python/python.exp
> +++ b/gdb/testsuite/gdb.python/python.exp
> @@ -417,3 +417,47 @@ gdb_py_test_silent_cmd "step" "Step into func2" 1
>  gdb_py_test_silent_cmd "up" "Step out of func2" 1
>  
>  gdb_test "python print (gdb.find_pc_line(gdb.selected_frame().pc()).line > line)" "True" "Test find_pc_line with resume address"
> +
> +# gdb.is_in_prologue and gdb.is_in_epilogue:
> +
> +# Start with a fresh gdb.
> +clean_restart ${testfile}
> +
> +if ![runto_main] then {
> +    fail "Can't run to main"
> +    return 0
> +}

Any reason these new tests aren't in a separate file?

> +
> +# Go somewhere in the function body.
> +runto [gdb_get_line_number "Break at func2 call site."]
> +
> +# Get the current frame object.
> +gdb_py_test_silent_cmd "python selectedFrame = gdb.selected_frame()" \
> +    "Get the current frame object." 0

Lowercase, and no "." please.  Here and throughout.  Usual style is to
drop the "the"s to be more concise:

gdb_py_test_silent_cmd "python selectedFrame = gdb.selected_frame()" \
    "get current frame object" 0

> +
> +# Get an address inside the function body.
> +gdb_py_test_silent_cmd "python functionBodyAddress = selectedFrame.pc()" \
> +    "Get an address inside the function body." 0

gdb_py_test_silent_cmd "python functionBodyAddress = selectedFrame.pc()" \
    "get address inside function body" 0

etc.

> +
> +# Get the start address of the function.
> +gdb_py_test_silent_cmd "python functionStartAddress = long(selectedFrame.function().value().address)" \
> +    "Get the start address of the function." 0
> +
> +# Test the function's start address and an address somewhere inside the function body.
> +
> +# With functionStartAddress:
> +gdb_test "python print(gdb.is_in_prologue(functionStartAddress, functionStartAddress))" "True" \
> +    "Function start is in the prologue"
> +
> +gdb_test "python print(gdb.is_in_prologue(functionBodyAddress, functionStartAddress))" "False" \
> +    "The function body isn't in the prologue"
> +
> +# Without functionStartAddress:
> +gdb_test "python print(gdb.is_in_prologue(functionStartAddress))" "True" \
> +    "Function start is in the prologue"
> +
> +gdb_test "python print(gdb.is_in_prologue(functionBodyAddress))" "False" \
> +    "The function body isn't in the prologue (requires debug info to pass)"
> +
> +gdb_test "python print(gdb.is_in_epilogue(functionBodyAddress))" "False" \
> +    "The function body isn't in the epilogue (requires debug info to pass)"
> 

Thanks,
Pedro Alves