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

Martin Galvan
2014-10-23T17:36:31Z

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?

--

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