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

Message ID 544A68B1.9000909@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Oct. 24, 2014, 2:56 p.m. UTC
  On 10/23/2014 06:36 PM, Martin Galvan wrote:
>> > 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.  */

Design mistakes in the internal APIs shouldn't be exposed to a public
API.  I'd even suggest that whatever Python API we end up with, it'd
be good to make the internal API match it.

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

Not sure an exception makes sense given the function's
interface.  Say in the future another optional parameter is added.
What would you do then?  What of old code that passed in func_start
but not that new argument?  Those might not expect an exception.
So for the case of the new argument not being specified, we'd
have to return 1, which is right -- the PC _might_ be pointing
at a prologue.

But, how exactly were you planning using the gdb.is_in_prologue
function?  GDB itself doesn't use this to determine whether locals
are valid, only gdbarch_in_function_epilogue_p/gdb.is_in_epilogue.

I think we can just remove the in_prologue function entirely.
Looking at the code in GDB that makes use of this, all we find is this
one caller:

      if ((ecs->event_thread->control.step_over_calls == STEP_OVER_NONE)
	  || ((ecs->event_thread->control.step_range_end == 1)
	      && in_prologue (gdbarch, ecs->event_thread->prev_pc,
			      ecs->stop_func_start)))
	{
	  /* I presume that step_over_calls is only 0 when we're
	     supposed to be stepping at the assembly language level
	     ("stepi").  Just stop.  */
	  /* Also, maybe we just did a "nexti" inside a prolog, so we
	     thought it was a subroutine call but it was not.  Stop as
	     well.  FENN */
	  /* And this works the same backward as frontward.  MVS */
	  end_stepping_range (ecs);
	  return;
	}

So this is only used for "nexti", and it's itself a dubious use.
It looks like old code papering over unwinder problems.

Doing some archaeology, I found the revision that code
was added:

 commit 100a02e1deec2f037a15cdf232f026dc79763bf8
 Author:     Andrew Cagney <cagney@redhat.com>
 AuthorDate: Thu Jun 28 21:48:41 2001 +0000
 Commit:     Andrew Cagney <cagney@redhat.com>
 CommitDate: Thu Jun 28 21:48:41 2001 +0000

     From Fernando Nasser:
     * infrun.c (handle_inferior_event): Handle "nexti" inside function
     prologues.

And the mailing list thread:

  https://sourceware.org/ml/gdb-patches/2001-01/msg00047.html

Not much discussion there, and no test...

Looking at the code around what was patched in that revision, we see
that the checks that detect whether the program has just stepped into a
subroutine didn't rely on the unwinders _at all_ back then!

From 'git show 100a02e1:gdb/infrun.c':

    if (stop_pc == ecs->stop_func_start         /* Quick test */
        || (in_prologue (stop_pc, ecs->stop_func_start) &&
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
            !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
        || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
        || ecs->stop_func_name == 0)
      {
        /* It's a subroutine call.  */

        if ((step_over_calls == STEP_OVER_NONE)
            || ((step_range_end == 1)
                && in_prologue (prev_pc, ecs->stop_func_start)))
          {
            /* I presume that step_over_calls is only 0 when we're
               supposed to be stepping at the assembly language level
               ("stepi").  Just stop.  */
            /* Also, maybe we just did a "nexti" inside a prolog,
               so we thought it was a subroutine call but it was not.
               Stop as well.  FENN */
            stop_step = 1;
            print_stop_reason (END_STEPPING_RANGE, 0);
            stop_stepping (ecs);
            return;
          }


Stripping the IN_SOLIB_RETURN_TRAMPOLINE checks for simplicity, we had:

    if (stop_pc == ecs->stop_func_start         /* Quick test */
        || in_prologue (stop_pc, ecs->stop_func_start)
        || ecs->stop_func_name == 0)
      {
        /* It's a subroutine call.  */

That's it.  Detecting a subroutine call was based on prologue
detection.  And that's why nexti needed the fix it needed back
then.  IOW, the in_prologue check in the current tree was undoing
a bad decision the in_prologue check just above did...

Compare to today's subroutine call check:

  /* Check for subroutine calls.  The check for the current frame
     equalling the step ID is not necessary - the check of the
     previous frame's ID is sufficient - but it is a common case and
     cheaper than checking the previous frame's ID.

     NOTE: frame_id_eq will never report two invalid frame IDs as
     being equal, so to get into this block, both the current and
     previous frame must have valid frame IDs.  */
  /* The outer_frame_id check is a heuristic to detect stepping
     through startup code.  If we step over an instruction which
     sets the stack pointer from an invalid value to a valid value,
     we may detect that as a subroutine call from the mythical
     "outermost" function.  This could be fixed by marking
     outermost frames as !stack_p,code_p,special_p.  Then the
     initial outermost frame, before sp was valid, would
     have code_addr == &_start.  See the comment in frame_id_eq
     for more.  */
  if (!frame_id_eq (get_stack_frame_id (frame),
		    ecs->event_thread->control.step_stack_frame_id)
      && (frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
		       ecs->event_thread->control.step_stack_frame_id)
	  && (!frame_id_eq (ecs->event_thread->control.step_stack_frame_id,
			    outer_frame_id)
	      || step_start_function != find_pc_function (stop_pc))))
    {

Nothing to do with prologues.  Completely relying on frame ids,
which are stable throughout the function.

It seems to me we can just remove the in_prologue check for nexti,
and the whole in_prologue function along with it...

I nexti'd manually a prologue for smoke testing, and it passes
regression testing for me on x86_64 Fedora 20, FWIW...

--------
From 822267fed3994acf191071653d10caf4c9dd7247 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 24 Oct 2014 14:49:33 +0100
Subject: [PATCH] Revert nexti prologue check

commit 100a02e1deec2f037a15cdf232f026dc79763bf8
Author:     Andrew Cagney <cagney@redhat.com>
AuthorDate: Thu Jun 28 21:48:41 2001 +0000
Commit:     Andrew Cagney <cagney@redhat.com>
CommitDate: Thu Jun 28 21:48:41 2001 +0000

    From Fernando Nasser:
    * infrun.c (handle_inferior_event): Handle "nexti" inside function
    prologues.
---
 gdb/infrun.c |  5 +----
 gdb/symtab.c | 73 ------------------------------------------------------------
 gdb/symtab.h |  3 ---
 3 files changed, 1 insertion(+), 80 deletions(-)
  

Comments

Martin Galvan Oct. 24, 2014, 7:49 p.m. UTC | #1
On Fri, Oct 24, 2014 at 11:56 AM, Pedro Alves <palves@redhat.com> wrote:
> On 10/23/2014 06:36 PM, Martin Galvan wrote:
>>> > 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.  */
>
> Design mistakes in the internal APIs shouldn't be exposed to a public
> API.  I'd even suggest that whatever Python API we end up with, it'd
> be good to make the internal API match it.
>
>>
>> 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.
>
> Not sure an exception makes sense given the function's
> interface.  Say in the future another optional parameter is added.
> What would you do then?  What of old code that passed in func_start
> but not that new argument?  Those might not expect an exception.
> So for the case of the new argument not being specified, we'd
> have to return 1, which is right -- the PC _might_ be pointing
> at a prologue.

I probably didn't make myself clear-- I wasn't talking about using
in_prologue directly anymore, but to follow its approach in the API
function. Of course it wouldn't make sense to put Python exception
raising directly inside in_prologue.

> But, how exactly were you planning using the gdb.is_in_prologue
> function?  GDB itself doesn't use this to determine whether locals
> are valid, only gdbarch_in_function_epilogue_p/gdb.is_in_epilogue.

Well, I followed the code while testing a rather simple function and
noticed that handle_step_into_function is very similar (in terms of
the approach) to in_prologue plus some address corrections and setting
a breakpoint to proceed to. The API function needs only the address
calculation part.

What if:
   1) I split handle_step_into_function in the address calc part and
the brakpoint insertion part,
moving the address calc to a new function (publicly available from infrun.h).
   2) I expose such function to the Python API.

Would that be accepted? Would you want to see a patch?

Please keep in mind that what I actually need is not really messing
with the prologue, but to know where the local variables are
accessible. If I could simply use DWARF info to accomplish that then I
wouldn't even touch the prologue at all.

Thanks!
  
Pedro Alves Oct. 24, 2014, 8:09 p.m. UTC | #2
On 10/24/2014 08:49 PM, Martin Galvan wrote:
> On Fri, Oct 24, 2014 at 11:56 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 10/23/2014 06:36 PM, Martin Galvan wrote:
>>>>> 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.  */
>>
>> Design mistakes in the internal APIs shouldn't be exposed to a public
>> API.  I'd even suggest that whatever Python API we end up with, it'd
>> be good to make the internal API match it.
>>
>>>
>>> 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.
>>
>> Not sure an exception makes sense given the function's
>> interface.  Say in the future another optional parameter is added.
>> What would you do then?  What of old code that passed in func_start
>> but not that new argument?  Those might not expect an exception.
>> So for the case of the new argument not being specified, we'd
>> have to return 1, which is right -- the PC _might_ be pointing
>> at a prologue.
> 
> I probably didn't make myself clear-- I wasn't talking about using
> in_prologue directly anymore, but to follow its approach in the API
> function. Of course it wouldn't make sense to put Python exception
> raising directly inside in_prologue.

That concern with about clients of the Python API, and if another
optional parameter is added to the Python API.

> 
>> But, how exactly were you planning using the gdb.is_in_prologue
>> function?  GDB itself doesn't use this to determine whether locals
>> are valid, only gdbarch_in_function_epilogue_p/gdb.is_in_epilogue.
> 
> Well, I followed the code while testing a rather simple function and
> noticed that handle_step_into_function is very similar (in terms of
> the approach) to in_prologue plus some address corrections and setting
> a breakpoint to proceed to. The API function needs only the address
> calculation part.
> 
> What if:
>    1) I split handle_step_into_function in the address calc part and
> the brakpoint insertion part,
> moving the address calc to a new function (publicly available from infrun.h).
>    2) I expose such function to the Python API.
> 
> Would that be accepted? Would you want to see a patch?
> 
> Please keep in mind that what I actually need is not really messing
> with the prologue, but to know where the local variables are
> accessible. If I could simply use DWARF info to accomplish that then I
> wouldn't even touch the prologue at all.

Hmm, how is this different from simply doing "break function" ?
GDB sets function breakpoints after the prologue already.  A "step"
into a function should stop at the exact same address as if the user
did "b function; c" to run to said function.

So, when you detect that you stepped into a function, you could
just set the breakpoint by function name?

Thanks,
Pedro Alves
  
Martin Galvan Oct. 24, 2014, 9:11 p.m. UTC | #3
On Fri, Oct 24, 2014 at 5:09 PM, Pedro Alves <palves@redhat.com> wrote:
>> Well, I followed the code while testing a rather simple function and
>> noticed that handle_step_into_function is very similar (in terms of
>> the approach) to in_prologue plus some address corrections and setting
>> a breakpoint to proceed to. The API function needs only the address
>> calculation part.
>>
>> What if:
>>    1) I split handle_step_into_function in the address calc part and
>> the brakpoint insertion part,
>> moving the address calc to a new function (publicly available from infrun.h).
>>    2) I expose such function to the Python API.
>>
>> Would that be accepted? Would you want to see a patch?
>>
>> Please keep in mind that what I actually need is not really messing
>> with the prologue, but to know where the local variables are
>> accessible. If I could simply use DWARF info to accomplish that then I
>> wouldn't even touch the prologue at all.
>
> Hmm, how is this different from simply doing "break function" ?
> GDB sets function breakpoints after the prologue already.  A "step"
> into a function should stop at the exact same address as if the user
> did "b function; c" to run to said function.
>
> So, when you detect that you stepped into a function, you could
> just set the breakpoint by function name?

In order for that to work, I'd have to run the program up to that
point. I really need to be able to determine if at a given PC the
local variables will be accessible without actually running the
program. Ideally I'd use only DWARF info to know that.

I looked up the approach GDB takes when setting a breakpoint at a
function name. From what I saw it appears to be similar as the
"optimistic" path from in_prologue (that is, using symtab and line
info). I guess that makes sense since setting a breakpoint by function
name by definition requires us to have debugging info.
  
Pedro Alves Oct. 24, 2014, 10:34 p.m. UTC | #4
On 10/24/2014 10:11 PM, Martin Galvan wrote:
> On Fri, Oct 24, 2014 at 5:09 PM, Pedro Alves <palves@redhat.com> wrote:
>>> Well, I followed the code while testing a rather simple function and
>>> noticed that handle_step_into_function is very similar (in terms of
>>> the approach) to in_prologue plus some address corrections and setting
>>> a breakpoint to proceed to. The API function needs only the address
>>> calculation part.
>>>
>>> What if:
>>>    1) I split handle_step_into_function in the address calc part and
>>> the brakpoint insertion part,
>>> moving the address calc to a new function (publicly available from infrun.h).
>>>    2) I expose such function to the Python API.
>>>
>>> Would that be accepted? Would you want to see a patch?
>>>
>>> Please keep in mind that what I actually need is not really messing
>>> with the prologue, but to know where the local variables are
>>> accessible. If I could simply use DWARF info to accomplish that then I
>>> wouldn't even touch the prologue at all.
>>
>> Hmm, how is this different from simply doing "break function" ?
>> GDB sets function breakpoints after the prologue already.  A "step"
>> into a function should stop at the exact same address as if the user
>> did "b function; c" to run to said function.
>>
>> So, when you detect that you stepped into a function, you could
>> just set the breakpoint by function name?
> 
> In order for that to work, I'd have to run the program up to that
> point. 

You can set breakpoints before running the program:

(top-gdb) b *main
Breakpoint 3 at 0x45ed30: file /home/pedro/gdb/mygit/build/../src/gdb/gdb.c, line 25.
(top-gdb) b main
Breakpoint 4 at 0x45ed3f: file /home/pedro/gdb/mygit/build/../src/gdb/gdb.c, line 28.

That offset is your "prologue", however meaningful that is.

> I really need to be able to determine if at a given PC the
> local variables will be accessible

Why?

> without actually running the
> program. Ideally I'd use only DWARF info to know that.

I think we still don't know what you're trying to do, only
a bit of _how_ you're trying to do it.  :-)  It makes it
harder to understand the use case, and to suggest solutions.

> I looked up the approach GDB takes when setting a breakpoint at a
> function name. From what I saw it appears to be similar as the
> "optimistic" path from in_prologue (that is, using symtab and line
> info). I guess that makes sense since setting a breakpoint by function
> name by definition requires us to have debugging info.

If you need access to local variables, then you're already
relying on debug info.

Thanks,
Pedro Alves
  
Martin Galvan Oct. 27, 2014, 4:40 p.m. UTC | #5
On Fri, Oct 24, 2014 at 7:34 PM, Pedro Alves <palves@redhat.com> wrote:
> You can set breakpoints before running the program:
>
> (top-gdb) b *main
> Breakpoint 3 at 0x45ed30: file /home/pedro/gdb/mygit/build/../src/gdb/gdb.c, line 25.
> (top-gdb) b main
> Breakpoint 4 at 0x45ed3f: file /home/pedro/gdb/mygit/build/../src/gdb/gdb.c, line 28.
>
> That offset is your "prologue", however meaningful that is.

Indeed. However, setting breakpoints just to parse that output is ugly :)

> I think we still don't know what you're trying to do, only
> a bit of _how_ you're trying to do it.  :-)  It makes it
> harder to understand the use case, and to suggest solutions.

In my case, I'm working on a Gdb-based fault injection machine which
will select some random PC from a given program, check if local
variables are visible at that point, drive the target to said point
and corrupt the contents of said variables. If I had a way to know at
which point a function's arguments will be ready to be used, the
attacks could be more effective: it's useless to corrupt the temporary
value the args have before they're initialized in the function's
prologue. However, as the PC is selected at random and it won't always
target the prologue, I wouldn't mind a few "missed" attacks if you
really don't want to include the prologue function.

The epilogue is a bit trickier: after the stack frame is destroyed,
Gdb will show the variables' addresses as pointing to other places in
the stack. If I were to corrupt those places, I could change the value
of the previous PC (which was stored as we entered the function),
which would make the program crash. While that seems like a desirable
thing, it's not for what I'm trying to test with this particular
behavior.

>> I looked up the approach GDB takes when setting a breakpoint at a
>> function name. From what I saw it appears to be similar as the
>> "optimistic" path from in_prologue (that is, using symtab and line
>> info). I guess that makes sense since setting a breakpoint by function
>> name by definition requires us to have debugging info.
>
> If you need access to local variables, then you're already
> relying on debug info.

Yes, but here's the thing: what in_prologue (and
handle_step_into_function, etc) do is taking the first address of a
function, getting its line number info and assuming the prologue ends
at the last address covered by that line. This is based on the
assumption that the compiler will mark the prologue as its own "line".
What confused me initially was that in_prologue had what appeared to
be a check for the case where a function would have all its code on a
single line. If that check didn't pass, it called
gdbarch_skip_prologue (which is what we're trying to avoid). I didn't
see something like that for the breakpoint setting code, so I tried
doing "break myFunction", where myFunction is defined in a single
line. It worked as expected: the breakpoint was still set at the end
of the prologue. I'm not certain, however, of how would this work on
hand-written assembly or the case where a compiler would schedule an
instruction from the function body to be inside the prologue (would it
mark that instruction as part of the first "line"?).

The bottom line is: I'm willing to drop the prologue API function, as
it's not essential to me particularly, but if we were to keep it we'd
have to require the function_start argument. I really need to keep the
epilogue (or perhaps I should say "stack destroyed"?) function,
though.
  

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index eca8eec..7a7d10d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4945,10 +4945,7 @@  process_event_stop_test (struct execution_control_state *ecs)
       if (debug_infrun)
 	 fprintf_unfiltered (gdb_stdlog, "infrun: stepped into subroutine\n");

-      if ((ecs->event_thread->control.step_over_calls == STEP_OVER_NONE)
-	  || ((ecs->event_thread->control.step_range_end == 1)
-	      && in_prologue (gdbarch, ecs->event_thread->prev_pc,
-			      ecs->stop_func_start)))
+      if (ecs->event_thread->control.step_over_calls == STEP_OVER_NONE)
 	{
 	  /* I presume that step_over_calls is only 0 when we're
 	     supposed to be stepping at the assembly language level
diff --git a/gdb/symtab.c b/gdb/symtab.c
index c530d50..f39fc6c 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3034,79 +3034,6 @@  skip_prologue_sal (struct symtab_and_line *sal)
     }
 }

-/* Determine if PC is in the prologue of a function.  The prologue is the area
-   between the first instruction of a function, and the first executable line.
-   Returns 1 if PC *might* be in prologue, 0 if definately *not* in prologue.
-
-   If non-zero, func_start is where we think the prologue starts, possibly
-   by previous examination of symbol table information.  */
-
-int
-in_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR func_start)
-{
-  struct symtab_and_line sal;
-  CORE_ADDR func_addr, func_end;
-
-  /* We have several sources of information we can consult to figure
-     this out.
-     - Compilers usually emit line number info that marks the prologue
-       as its own "source line".  So the ending address of that "line"
-       is the end of the prologue.  If available, this is the most
-       reliable method.
-     - The minimal symbols and partial symbols, which can usually tell
-       us the starting and ending addresses of a function.
-     - If we know the function's start address, we can call the
-       architecture-defined gdbarch_skip_prologue function to analyze the
-       instruction stream and guess where the prologue ends.
-     - Our `func_start' argument; if non-zero, this is the caller's
-       best guess as to the function's entry point.  At the time of
-       this writing, handle_inferior_event doesn't get this right, so
-       it should be our last resort.  */
-
-  /* Consult the partial symbol table, to find which function
-     the PC is in.  */
-  if (! find_pc_partial_function (pc, NULL, &func_addr, &func_end))
-    {
-      CORE_ADDR prologue_end;
-
-      /* 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.  */
-
-      prologue_end = gdbarch_skip_prologue (gdbarch, func_start);
-
-      return func_start <= pc && pc < prologue_end;
-    }
-
-  /* If we have line number information for the function, that's
-     usually pretty reliable.  */
-  sal = find_pc_line (func_addr, 0);
-
-  /* Now sal describes the source line at the function's entry point,
-     which (by convention) is the prologue.  The end of that "line",
-     sal.end, is the end of the prologue.
-
-     Note that, for functions whose source code is all on a single
-     line, the line number information doesn't always end up this way.
-     So we must verify that our purported end-of-prologue address is
-     *within* the function, not at its start or end.  */
-  if (sal.line == 0
-      || sal.end <= func_addr
-      || func_end <= sal.end)
-    {
-      /* We don't have any good line number info, so use the minsym
-	 information, together with the architecture-specific prologue
-	 scanning code.  */
-      CORE_ADDR prologue_end = gdbarch_skip_prologue (gdbarch, func_addr);
-
-      return func_addr <= pc && pc < prologue_end;
-    }
-
-  /* We have line number info, and it looks good.  */
-  return func_addr <= pc && pc < sal.end;
-}
-
 /* Given PC at the function's start address, attempt to find the
    prologue end using SAL information.  Return zero if the skip fails.

diff --git a/gdb/symtab.h b/gdb/symtab.h
index 4a47d48..aaef9a0 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1320,9 +1320,6 @@  extern enum language deduce_language_from_filename (const char *);

 /* symtab.c */

-extern int in_prologue (struct gdbarch *gdbarch,
-			CORE_ADDR pc, CORE_ADDR func_start);
-
 extern CORE_ADDR skip_prologue_using_sal (struct gdbarch *gdbarch,
 					  CORE_ADDR func_addr);