Python API: Add gdb.is_in_prologue and gdb.is_in_epilogue.

  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.

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.

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(+)
  

> From: Martin Galvan <martin.galvan@tallertechnologies.com>
> Cc: Martin Galvan <martin.galvan@tallertechnologies.com>
> Date: Wed, 22 Oct 2014 11:01:25 -0300
> 
> Added two new functions to the Python API: gdb.is_in_prologue and gdb.is_in_epilogue.

Thanks.

>     * NEWS: Mention new Python functions.

This part is OK.

> +@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

Please use Texinfo markup for emphasis, not ASCII art.  I think you
want @emph{might} here.

> +The optional @var{functionStart} argument is the start address of the
> +function you want to check if @var{pc} belongs to.  If your binary
                              ^^
"whether"

> +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
            ^^
Likewise.

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

Isn't it better to require this argument in that case?  Zero is not
very useful starting address, in most cases.

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

This repeats almost verbatim what has been already said about the
prologue.  It would be better to make a single description that covers
both the prologue and the epilogue, before you describe the 2 methods.

OK with those changes.
  

On Wed, Oct 22, 2014 at 12:10 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> OK with those changes.

Awesome, will send v2 right away.
  

On Wed, Oct 22, 2014 at 12:10 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Martin Galvan <martin.galvan@tallertechnologies.com>
>> Cc: Martin Galvan <martin.galvan@tallertechnologies.com>
>> +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.
>
> Isn't it better to require this argument in that case?  Zero is not
> very useful starting address, in most cases.

I looked at this again and I think it should stay as an optional
argument. GDB's in_prologue will only check for the functionStart
argument when it has no other way to determine whether the given PC
belongs to a prologue; otherwise it'll just ignore it. Because of
this, is_in_prologue will return True if we pass it a pc that belongs
to a prologue together with any valid address regardless of it being a
function start, let alone the function PC belongs to. Making it a
required argument to me sounds like we're asking is_in_prologue
whether PC is in the prologue of the function starting at
functionStart, instead of simply telling whether PC belongs to any
prologue (which was my original intent).

I thought of removing the functionStart argument altogether, however
if we're working without debugging info but we're certain of where the
function starts we can still get an accurate result instead of a false
positive. Let me know what you think of this so I can send v2 (I'll
also include a few minor corrections such as correctly casting
gdbpy_is_in_prologue to PyCFunction in the method table).
  

> Date: Wed, 22 Oct 2014 14:33:15 -0300
> From: Martin Galvan <martin.galvan@tallertechnologies.com>
> Cc: gdb-patches@sourceware.org
> 
> On Wed, Oct 22, 2014 at 12:10 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> >> From: Martin Galvan <martin.galvan@tallertechnologies.com>
> >> Cc: Martin Galvan <martin.galvan@tallertechnologies.com>
> >> +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.
> >
> > Isn't it better to require this argument in that case?  Zero is not
> > very useful starting address, in most cases.
> 
> I looked at this again and I think it should stay as an optional
> argument. GDB's in_prologue will only check for the functionStart
> argument when it has no other way to determine whether the given PC
> belongs to a prologue; otherwise it'll just ignore it. Because of
> this, is_in_prologue will return True if we pass it a pc that belongs
> to a prologue together with any valid address regardless of it being a
> function start, let alone the function PC belongs to. Making it a
> required argument to me sounds like we're asking is_in_prologue
> whether PC is in the prologue of the function starting at
> functionStart, instead of simply telling whether PC belongs to any
> prologue (which was my original intent).

I think we are miscommunicating.  What I had in mind is raise an
exception or display an error message when GDB has no other means to
determine where the function's start is (e.g., no debug info), and no
functionStart argument was passed.  That is what I meant by
"require".  IOW, it's up to the user to decide when to provide it, but
GDB will refuse to use some arbitrary number, such as zero, if it
cannot determine the starting address.

(Btw, using mixed-case argument names in thge manual is a bad idea,
because in the Info format @var causes its argument to be up-cased.)
  

On Wed, Oct 22, 2014 at 2:47 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> I think we are miscommunicating.  What I had in mind is raise an
> exception or display an error message when GDB has no other means to
> determine where the function's start is (e.g., no debug info), and no
> functionStart argument was passed.  That is what I meant by
> "require".  IOW, it's up to the user to decide when to provide it, but
> GDB will refuse to use some arbitrary number, such as zero, if it
> cannot determine the starting address.

Oh, I see. That's a great idea.

I think the simplest way to do this is not to call GDB's in_prologue
directly from gdbpy_is_in_prologue, but reproduce some of its behavior
and adding a check on the return value of find_pc_partial_function
which will throw an exception if functionStart wasn't provided. This
may result in a bit of duplicated code, but in_prologue isn't that
long a function if you take the comments out so I don't think that
would be a problem. What do you think?

> (Btw, using mixed-case argument names in thge manual is a bad idea,
> because in the Info format @var causes its argument to be up-cased.)

Indeed, will also change that.

Thanks a lot!
  

> Date: Wed, 22 Oct 2014 20:47:08 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gdb-patches@sourceware.org
> 
> I think we are miscommunicating.  What I had in mind is raise an
> exception or display an error message when GDB has no other means to
> determine where the function's start is (e.g., no debug info), and no
> functionStart argument was passed.  That is what I meant by
> "require".  IOW, it's up to the user to decide when to provide it, but
> GDB will refuse to use some arbitrary number, such as zero, if it
> cannot determine the starting address.

Having said all that, please don't read this as a rejection of your
code.  Even if you agree with me, please wait for others to chime in
and say what they think, before acting on my opinions.

Thanks.
  

On Wed, Oct 22, 2014 at 3:07 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> Having said all that, please don't read this as a rejection of your
> code.  Even if you agree with me, please wait for others to chime in
> and say what they think, before acting on my opinions.
>
> Thanks.

Don't worry about that.

Looking at the documentation again, I think saying "functionStart [..]
is the start address of the function you want to check if pc belongs
to" is a bit misleading in this case. I think that paragraph should be
changed to something like:

"The optional @var{functionStart} argument is the start address of the
function @var{pc} belongs to.  If your binary doesn't have debugging info,
@value{GDBN} will need to use this value to guess whether @var{pc}
belongs to the
prologue, otherwise it'll ignore it. Notice that in the latter case you can pass
any valid address as @var{functionStart}; the result will only depend
on @var{pc}
being in a prologue, even if it's not the prologue of the function starting at
@var{functionStart}."
  

> Date: Wed, 22 Oct 2014 15:32:38 -0300
> From: Martin Galvan <martin.galvan@tallertechnologies.com>
> Cc: gdb-patches@sourceware.org
> 
> "The optional @var{functionStart} argument is the start address of the
> function @var{pc} belongs to.  If your binary doesn't have debugging info,
> @value{GDBN} will need to use this value to guess whether @var{pc}
> belongs to the                           ^^^^^^^^
> prologue, otherwise it'll ignore it. Notice that in the latter case you can pass
> any valid address as @var{functionStart}; the result will only depend
> on @var{pc}
> being in a prologue, even if it's not the prologue of the function starting at
> @var{functionStart}."

I'd say "to determine" instead of "to guess"/

Otherwise, fine with me, thanks.  (But please remember to leave 2
spaces between sentences.)
  

Martin Galvan writes:
 > 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.
 > 
 > 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.

Hi.

We should have a test for is_in_epilogue anyway.
The canonical choice is to just make it amd64-linux specific.

API functions like these are problematic.
Users don't expect API functions to be heuristic-based,
and that is all these can ever be in the general case.
The patch does try to provide the user some guarantees
("however if the result is @code{False} you can be sure
@value{GDBN} is right."),
but it's not clear to me this will be true in the general case.
I may have missed something so I'm certainly willing to be
persuaded otherwise.
I might be ok with this patch if the functions were named something like
"maybe_is_in_prologue" and "maybe_is_in_epilogue".
That way they scream to the user (and future code readers) "Heads Up!"
  

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
  

On 10/22/2014 10:34 PM, Pedro Alves wrote:
>> +# 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
           ^^^^^^^

BTW, I happened to read this back, and noticed: "current" and "selected"
have distinct meanings in the frame machinery.  Best not mix them up:

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

Thanks,
Pedro Alves
  

First of all, thanks a lot to all of you for the feedback.

On Wed, Oct 22, 2014 at 4:23 PM, Doug Evans <dje@google.com> wrote:
> We should have a test for is_in_epilogue anyway.
> The canonical choice is to just make it amd64-linux specific.

I briefly thought of that. The test I originally wrote for
is_in_epilogue checked for the last PC of a known, simple function
which ended with retq on amd64 and pop on thumb ARM. It worked fine on
ARM, however for some reason the retq was returning False. I looked at
amd64_in_function_epilogue_p and saw all it did was check if the
opcode was the one for ret. Not being too familiar with amd64 I
initially assumed the opcode for retq was different, but after doing
an objdump I noticed it's the same (0xc3). I was surprised to see
this, and debugged GDB itself only to find out that
gdbarch->in_function_epilogue_p was the generic one instead of amd64.
Is this a bug?

> API functions like these are problematic.
> Users don't expect API functions to be heuristic-based,
> and that is all these can ever be in the general case.
> The patch does try to provide the user some guarantees
> ("however if the result is @code{False} you can be sure
> @value{GDBN} is right."),
> but it's not clear to me this will be true in the general case.
> I may have missed something so I'm certainly willing to be
> persuaded otherwise.

Well, the comment at the top of in_prologue says "Returns 1 if PC
*might* be in prologue, 0 if definately (sic) *not* in prologue".
However, looking at the function itself it relies on the compiler
having marked the prologue as its own "line", and if it can't use that
info it falls back to using the architecture-dependant
gdbarch_skip_prologue. So far every time I've tested it it's worked
fine, and if it didn't it would've probably already been reported as a
bug of in_prologue or gdbarch_skip_prologue. I guess if you want to be
*really* careful I could change that line in the documentation for
something like "if the result is False, you can be almost sure GDB is
right", or "GDB is most likely right".

I think in this case it's more important to emphasize false positives
rather than false negatives.

> I might be ok with this patch if the functions were named something like
> "maybe_is_in_prologue" and "maybe_is_in_epilogue".
> That way they scream to the user (and future code readers) "Heads Up!"

Agreed, although from what Pedro said I think it would be even more
accurate if we renamed the epilogue function to something like
"stack_frame_destroyed".

On Wed, Oct 22, 2014 at 6:34 PM, Pedro Alves <palves@redhat.com> wrote:
> 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.

Agreed.

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

I actually wrote a test that checked is_in_epilogue with the last PC
in a very simple function. I used it to test on ARM and it worked
fine, however for amd64 the story was different (see my answer to
Doug), and that's why I didn't include it.

If we were to single-step and do architecture-dependant checks for
every single instruction, however, I think the test would be quite
complicated.

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

Agreed. How about:

** Two new functions: gdb.maybe_in_prologue and gdb.stack_destroyed.
    The former returns True if a given PC may be inside a function's prologue
    and thus the local variables may show wrong values at that point,
    and the latter returns True if a given PC is at a point where GDB detected
    the stack frame as having been destroyed, usually by the instructions
    in a function's epilogue.

>> +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?

I used 0 because in_prologue expects it to be non-zero. If it's 0 and
we have no debugging info, it'll always return true:

      /* We don't even have minsym information, so fall back to using
         func_start, if given.  */
    if (! func_start)
        return 1;        /* We *might* be in a prologue.  */

Again, I did it because of the way in_prologue works, but as Eli said
this would probably be better handled with a Python exception or a
message of some kind.

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

Not particularly. I thought they should go in python.exp since my
functions belong to the gdb module itself, but I could place them in a
separate file if you want. I'm thinking of naming the file
py-prologue-epilogue.exp or maybe py-frame-is-invalid.exp. Any better
naming suggestions are welcome!

>> +# 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
>
> BTW, I happened to read this back, and noticed: "current" and "selected"
> have distinct meanings in the frame machinery.  Best not mix them up:
>
> gdb_py_test_silent_cmd "python selectedFrame = gdb.selected_frame()" \
>     "get selected frame object" 0

Will do.

One more thing: I'd like to know if everyone's ok with this:

On Wed, Oct 22, 2014 at 3:06 PM, Martin Galvan
<martin.galvan@tallertechnologies.com> wrote:
> On Wed, Oct 22, 2014 at 2:47 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> I think we are miscommunicating.  What I had in mind is raise an
>> exception or display an error message when GDB has no other means to
>> determine where the function's start is (e.g., no debug info), and no
>> functionStart argument was passed.  That is what I meant by
>> "require".  IOW, it's up to the user to decide when to provide it, but
>> GDB will refuse to use some arbitrary number, such as zero, if it
>> cannot determine the starting address.
>
> Oh, I see. That's a great idea.
>
> I think the simplest way to do this is not to call GDB's in_prologue
> directly from gdbpy_is_in_prologue, but reproduce some of its behavior
> and adding a check on the return value of find_pc_partial_function
> which will throw an exception if functionStart wasn't provided. This
> may result in a bit of duplicated code, but in_prologue isn't that
> long a function if you take the comments out so I don't think that
> would be a problem. What do you think?
  

On Thu, Oct 23, 2014 at 2:57 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> The fundamental problem is that the notion of "prologue" and "epilogue"
> simply no longer exists in optimized code generated by modern compilers;
> and even more compiler features get implemented that make those notions
> even less useful (e.g. shrink-wrapping).
>
> As a result, we have been trying to the rid of using those notions as
> much as possible; for example, when debugging optimized code with modern
> DWARF information present, GDB will today no longer even use prologue
> skipping at all.  Instead, the debug information is good enough that
> the correct location of local variables can be recovered at every
> instruction in the function, making the distinction no longer needed.
>
> The in_prologue routine is likewise only still uses under certain rather
> rare circumstances; in fact it might even today be possible to simply
> remove it.  Once more platforms provide correct DWARF covering epilogues
> as well, the gdbarch_in_function_epilogue_p calls in breakpoint.c may
> likewise become unnecessary.
>
> So if we hope at some point to get rid of those routines, then it seems
> counterproductive to now export them as part of a fixed external API ...

While that may be true, it's also true that at some points we still
see the local variables having wrong values when stepping through
machine code. The aim of this patch is to expose a way of detecting
such situations for scripts that may need it. Until we have a safer
way to do it I think this should be integrated to the code base.
  

On Thu, Oct 23, 2014 at 3:09 PM, Martin Galvan
<martin.galvan@tallertechnologies.com> wrote:
> On Thu, Oct 23, 2014 at 2:57 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> The fundamental problem is that the notion of "prologue" and "epilogue"
>> simply no longer exists in optimized code generated by modern compilers;
>> and even more compiler features get implemented that make those notions
>> even less useful (e.g. shrink-wrapping).
>>
>> As a result, we have been trying to the rid of using those notions as
>> much as possible; for example, when debugging optimized code with modern
>> DWARF information present, GDB will today no longer even use prologue
>> skipping at all.  Instead, the debug information is good enough that
>> the correct location of local variables can be recovered at every
>> instruction in the function, making the distinction no longer needed.
>>
>> The in_prologue routine is likewise only still uses under certain rather
>> rare circumstances; in fact it might even today be possible to simply
>> remove it.  Once more platforms provide correct DWARF covering epilogues
>> as well, the gdbarch_in_function_epilogue_p calls in breakpoint.c may
>> likewise become unnecessary.
>>
>> So if we hope at some point to get rid of those routines, then it seems
>> counterproductive to now export them as part of a fixed external API ...
>
> While that may be true, it's also true that at some points we still
> see the local variables having wrong values when stepping through
> machine code. The aim of this patch is to expose a way of detecting
> such situations for scripts that may need it. Until we have a safer
> way to do it I think this should be integrated to the code base.

Hi all,
   (Hi Pedro!)

 we badly need this. If you think the patch is in a shape good enough
to be committed, please commit it for Martín since he doesn't have
write access.

We can then start a fresh new thread to discuss future directions
specially related to optimized code and exactly what/how DWARF
tags should be handled.

Thanks!

   Daniel.

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

On Thu, Oct 23, 2014 at 11:13 AM, Daniel Gutson
<daniel.gutson@tallertechnologies.com> wrote:
> On Thu, Oct 23, 2014 at 3:09 PM, Martin Galvan
> <martin.galvan@tallertechnologies.com> wrote:
>> On Thu, Oct 23, 2014 at 2:57 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>>> The fundamental problem is that the notion of "prologue" and "epilogue"
>>> simply no longer exists in optimized code generated by modern compilers;
>>> and even more compiler features get implemented that make those notions
>>> even less useful (e.g. shrink-wrapping).
>>>
>>> As a result, we have been trying to the rid of using those notions as
>>> much as possible; for example, when debugging optimized code with modern
>>> DWARF information present, GDB will today no longer even use prologue
>>> skipping at all.  Instead, the debug information is good enough that
>>> the correct location of local variables can be recovered at every
>>> instruction in the function, making the distinction no longer needed.
>>>
>>> The in_prologue routine is likewise only still uses under certain rather
>>> rare circumstances; in fact it might even today be possible to simply
>>> remove it.  Once more platforms provide correct DWARF covering epilogues
>>> as well, the gdbarch_in_function_epilogue_p calls in breakpoint.c may
>>> likewise become unnecessary.
>>>
>>> So if we hope at some point to get rid of those routines, then it seems
>>> counterproductive to now export them as part of a fixed external API ...
>>
>> While that may be true, it's also true that at some points we still
>> see the local variables having wrong values when stepping through
>> machine code. The aim of this patch is to expose a way of detecting
>> such situations for scripts that may need it. Until we have a safer
>> way to do it I think this should be integrated to the code base.
>
> Hi all,
>    (Hi Pedro!)
>
>  we badly need this. If you think the patch is in a shape good enough
> to be committed, please commit it for Martín since he doesn't have
> write access.
>
> We can then start a fresh new thread to discuss future directions
> specially related to optimized code and exactly what/how DWARF
> tags should be handled.

Ulrich raises a valid point though.
API design needs to be done with care.
I'd rather not rush this.
  

On Thu, Oct 23, 2014 at 10:36 AM, Martin Galvan
<martin.galvan@tallertechnologies.com> wrote:
> First of all, thanks a lot to all of you for the feedback.
>
> On Wed, Oct 22, 2014 at 4:23 PM, Doug Evans <dje@google.com> wrote:
>> We should have a test for is_in_epilogue anyway.
>> The canonical choice is to just make it amd64-linux specific.
>
> I briefly thought of that. The test I originally wrote for
> is_in_epilogue checked for the last PC of a known, simple function
> which ended with retq on amd64 and pop on thumb ARM. It worked fine on
> ARM, however for some reason the retq was returning False. I looked at
> amd64_in_function_epilogue_p and saw all it did was check if the
> opcode was the one for ret. Not being too familiar with amd64 I
> initially assumed the opcode for retq was different, but after doing
> an objdump I noticed it's the same (0xc3). I was surprised to see
> this, and debugged GDB itself only to find out that
> gdbarch->in_function_epilogue_p was the generic one instead of amd64.
> Is this a bug?

Dunno offhand.
However, I still think there needs to be a testcase that exercises the
API call, however minimally.
[I don't care much about the details of the exercising in this
particular case, just that it is at least minimally exercised.  The
more exercise the better of course.]

>> API functions like these are problematic.
>> Users don't expect API functions to be heuristic-based,
>> and that is all these can ever be in the general case.
>> The patch does try to provide the user some guarantees
>> ("however if the result is @code{False} you can be sure
>> @value{GDBN} is right."),
>> but it's not clear to me this will be true in the general case.
>> I may have missed something so I'm certainly willing to be
>> persuaded otherwise.
>
> Well, the comment at the top of in_prologue says "Returns 1 if PC
> *might* be in prologue, 0 if definately (sic) *not* in prologue".
> However, looking at the function itself it relies on the compiler
> having marked the prologue as its own "line", and if it can't use that
> info it falls back to using the architecture-dependant
> gdbarch_skip_prologue.

Yep.

> So far every time I've tested it it's worked
> fine, and if it didn't it would've probably already been reported as a
> bug of in_prologue or gdbarch_skip_prologue.

Optimized code, and hand written assembler can make this problematic.

From the point of view of the user trying to use this function, if the
compiler moves an instruction into the middle of the prologue, what
should the function return when the pc is back in the prologue after
having left it?

E.g.,
my_function:
    prologue_insn_1
    random_instruction_scheduled_into_prologue
    prologue_insn_2   ; <<<<

The function will always return 0 for prologue_insn_2, but users not
familiar such things may find this confusing.

> I guess if you want to be
> *really* careful I could change that line in the documentation for
> something like "if the result is False, you can be almost sure GDB is
> right", or "GDB is most likely right".
>
> I think in this case it's more important to emphasize false positives
> rather than false negatives.

I don't have a problem with this.
Naming the function maybe_is_in_prologue helps emphasize "*might* be
in prologue".

>> I might be ok with this patch if the functions were named something like
>> "maybe_is_in_prologue" and "maybe_is_in_epilogue".
>> That way they scream to the user (and future code readers) "Heads Up!"
>
> Agreed, although from what Pedro said I think it would be even more
> accurate if we renamed the epilogue function to something like
> "stack_frame_destroyed".

If one went that route then I wonder whether we need two API functions.
[If we did go with only one function I'd choose a different name than
foo_destroyed of course.]
The problem being solved is wanting to know whether the debug
information can be used to look up local variables.  Is there another
problem being solved by knowing whether we're in the prologue?

> On Wed, Oct 22, 2014 at 6:34 PM, Pedro Alves <palves@redhat.com> wrote:
>> 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.
>
> Agreed.
>
>> 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.
>
> I actually wrote a test that checked is_in_epilogue with the last PC
> in a very simple function. I used it to test on ARM and it worked
> fine, however for amd64 the story was different (see my answer to
> Doug), and that's why I didn't include it.
>
> If we were to single-step and do architecture-dependant checks for
> every single instruction, however, I think the test would be quite
> complicated.

Not necessarily that complicated.
We have very explicit dwarf testcases to exercise gdb processing of
special dwarf conditions.
We *could* do a similar thing here (e.g., have an assembler function
with a known number of instructions and exercise the API call(s) at
each instruction).

>>> +  ** 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.
>
> Agreed. How about:
>
> ** Two new functions: gdb.maybe_in_prologue and gdb.stack_destroyed.
>     The former returns True if a given PC may be inside a function's prologue
>     and thus the local variables may show wrong values at that point,
>     and the latter returns True if a given PC is at a point where GDB detected
>     the stack frame as having been destroyed, usually by the instructions
>     in a function's epilogue.
>
>>> +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?
>
> I used 0 because in_prologue expects it to be non-zero. If it's 0 and
> we have no debugging info, it'll always return true:
>
>       /* We don't even have minsym information, so fall back to using
>          func_start, if given.  */
>     if (! func_start)
>         return 1;        /* We *might* be in a prologue.  */
>
> Again, I did it because of the way in_prologue works, but as Eli said
> this would probably be better handled with a Python exception or a
> message of some kind.
>
>> Any reason these new tests aren't in a separate file?
>
> Not particularly. I thought they should go in python.exp since my
> functions belong to the gdb module itself, but I could place them in a
> separate file if you want.

A separate file would be preferable.

> I'm thinking of naming the file
> py-prologue-epilogue.exp or maybe py-frame-is-invalid.exp. Any better
> naming suggestions are welcome!
>
>>> +# 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
>>
>> BTW, I happened to read this back, and noticed: "current" and "selected"
>> have distinct meanings in the frame machinery.  Best not mix them up:
>>
>> gdb_py_test_silent_cmd "python selectedFrame = gdb.selected_frame()" \
>>     "get selected frame object" 0
>
> Will do.
>
> One more thing: I'd like to know if everyone's ok with this:
>
> On Wed, Oct 22, 2014 at 3:06 PM, Martin Galvan
> <martin.galvan@tallertechnologies.com> wrote:
>> On Wed, Oct 22, 2014 at 2:47 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> I think we are miscommunicating.  What I had in mind is raise an
>>> exception or display an error message when GDB has no other means to
>>> determine where the function's start is (e.g., no debug info), and no
>>> functionStart argument was passed.  That is what I meant by
>>> "require".  IOW, it's up to the user to decide when to provide it, but
>>> GDB will refuse to use some arbitrary number, such as zero, if it
>>> cannot determine the starting address.
>>
>> Oh, I see. That's a great idea.
>>
>> I think the simplest way to do this is not to call GDB's in_prologue
>> directly from gdbpy_is_in_prologue, but reproduce some of its behavior
>> and adding a check on the return value of find_pc_partial_function
>> which will throw an exception if functionStart wasn't provided. This
>> may result in a bit of duplicated code, but in_prologue isn't that
>> long a function if you take the comments out so I don't think that
>> would be a problem. What do you think?

I prefer the robustness of flagging an exception over a heuristic in
the API, so yeah, works for me.
  

On 10/24/2014 05:57 AM, Doug Evans wrote:
> If one went that route then I wonder whether we need two API functions.
> [If we did go with only one function I'd choose a different name than
> foo_destroyed of course.]

Do you have a better suggestion for the gdbarch hook?  I think we
should just rename it for good, avoiding these confusions further.

Thanks,
Pedro Alves
  

On Fri, Oct 24, 2014 at 8:34 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Pedro Alves wrote:
>> On 10/24/2014 05:57 AM, Doug Evans wrote:
>> > If one went that route then I wonder whether we need two API functions.
>> > [If we did go with only one function I'd choose a different name than
>> > foo_destroyed of course.]
>>
>> Do you have a better suggestion for the gdbarch hook?  I think we
>> should just rename it for good, avoiding these confusions further.
>
> So if the only use of this interface is to check whether the result of
> some other interface (I assume something like Frame.read_var ?) is
> reliable, then I guess we might consider moving the check actually
> into that other interface.  E.g. have Frame.read_var itself check
> in_epilogue and return an unavailable or optimized-out value if
> the value would be unreliable otherwise.

I can imagine someone wanting to do the check before doing
gdb.parse_and_eval (an escape hatch for general expression
evaluation).

Also, FAOD, the API function in question still should check whether
the pc is in the prologue (unless, e.g., gdb knows the debug info is
usable) as there too locals may not be accessible.