Message ID | CAOKbPbaa2jZonzn-tcH9C8ge5AVUJHJeREwWNLOokFqr7dd6vw@mail.gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 4670 invoked by alias); 12 Nov 2014 15:55:19 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 4655 invoked by uid 89); 12 Nov 2014 15:55:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_SOFTFAIL autolearn=no version=3.3.2 X-HELO: mail-lb0-f173.google.com Received: from mail-lb0-f173.google.com (HELO mail-lb0-f173.google.com) (209.85.217.173) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 12 Nov 2014 15:55:16 +0000 Received: by mail-lb0-f173.google.com with SMTP id n15so9720273lbi.18 for <gdb-patches@sourceware.org>; Wed, 12 Nov 2014 07:55:12 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=/YWJcAj/r2ICrL/swLfP472LN41+NJih8Z4RbZ9qRRQ=; b=hTwSZiYzNwpLbMcJG+10Ho97XhLdYO6zaOMDfRHoHrlUwF8FIMZu/QtLsf9GMH02wx LNC1eNKrWkFeCrpMiAvF+ylRw/qiMrw2ddbdwedpowX7ZbZlJPwsMsutLGdeJKqCmUd8 A9yQSLWUIrk5XrRJjdzq6U/gDsmOiM5dnnEdjWrOtjyl5vx2ysLvQkUjJhTBNxFaSL2w XbO3itNp4Fmz6Uh2iZW2OyLxLJP7XDH3N4CY3H+ycKKaRiS4PXFTtsQkc8ZwHlsbtAKQ aPFEmCz4tfVPeQR3zDNMsJSdPAJeoU1Kayr5J9/IRFGCyE3NGNvZaUSZKRRsrNwQCVvh VPBA== X-Gm-Message-State: ALoCoQlxVtbz/VJTeQMMZYSdCtu8ctXvNr4e+sxPLSeTOfJtNc1XForh5x3vTcI+6myxT8vofwE+ MIME-Version: 1.0 X-Received: by 10.112.159.129 with SMTP id xc1mr43329637lbb.24.1415807712288; Wed, 12 Nov 2014 07:55:12 -0800 (PST) Received: by 10.112.59.129 with HTTP; Wed, 12 Nov 2014 07:55:12 -0800 (PST) In-Reply-To: <CAOKbPbY24zgvHdmAYQvR6H=sCsF6ixzwvwdbhPpBkcdcsMotiw@mail.gmail.com> References: <CAOKbPbZd+ppseGQW2OirBm4y5O=LgUMP-Pf8=RF00hnPOuMutw@mail.gmail.com> <201411071727.sA7HRNIQ007851@d03av02.boulder.ibm.com> <CAOKbPbY24zgvHdmAYQvR6H=sCsF6ixzwvwdbhPpBkcdcsMotiw@mail.gmail.com> Date: Wed, 12 Nov 2014 12:55:12 -0300 Message-ID: <CAOKbPbaa2jZonzn-tcH9C8ge5AVUJHJeREwWNLOokFqr7dd6vw@mail.gmail.com> Subject: Re: [PING][RFC][PATCH v2] Python API: add gdb.stack_may_be_invalid From: Martin Galvan <martin.galvan@tallertechnologies.com> To: Ulrich Weigand <uweigand@de.ibm.com> Cc: gdb-patches@sourceware.org, Doug Evans <dje@google.com>, Eli Zaretskii <eliz@gnu.org>, Pedro Alves <palves@redhat.com>, Daniel Gutson <daniel.gutson@tallertechnologies.com> Content-Type: text/plain; charset=UTF-8 |
Commit Message
Martin Galvan
Nov. 12, 2014, 3:55 p.m. UTC
On Fri, Nov 7, 2014 at 2:36 PM, Martin Galvan <martin.galvan@tallertechnologies.com> wrote: > On Fri, Nov 7, 2014 at 2:27 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote: >> Just one comment here: python_gdbarch isn't really correct here. >> If you have a platform that supports multiple architectures, then >> you really should use the appropriate gdbarch for PC. >> >> Ideally, the Python interface should carry enough information to >> determine the appropriate gdbarch, e.g. by operating on a Frame >> instead of a plain PC value. > > If I understand correctly, using a Frame would require the program to > be already running by the time we call the API function, which isn't > really what we want. > >> If that isn't possible, one fall-back might be to look up the >> symbol table from the PC, and use the associated objfile arch. Here's the new version of the patch. It uses the objfile's gdbarch and, if not available, python_gdbarch. static PyObject * @@ -2000,6 +2081,15 @@ Return the selected inferior object." }, { "inferiors", gdbpy_inferiors, METH_NOARGS, "inferiors () -> (gdb.Inferior, ...).\n\ Return a tuple containing all inferiors." }, + + + { "stack_may_be_invalid", gdbpy_stack_may_be_invalid, METH_VARARGS, + "stack_may_be_invalid (Long) -> Boolean.\n\ +Returns True if a given PC may point to an address in which the stack frame\n\ +may not be valid (either because it may not be set up yet or because it was\n\ +destroyed, usually in a function's epilogue), False otherwise."}, + + {NULL, NULL, 0, NULL} };
Comments
On Wed, Nov 12, 2014 at 7:55 AM, Martin Galvan <martin.galvan@tallertechnologies.com> wrote: > On Fri, Nov 7, 2014 at 2:36 PM, Martin Galvan > <martin.galvan@tallertechnologies.com> wrote: >> On Fri, Nov 7, 2014 at 2:27 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote: >>> Just one comment here: python_gdbarch isn't really correct here. >>> If you have a platform that supports multiple architectures, then >>> you really should use the appropriate gdbarch for PC. >>> >>> Ideally, the Python interface should carry enough information to >>> determine the appropriate gdbarch, e.g. by operating on a Frame >>> instead of a plain PC value. >> >> If I understand correctly, using a Frame would require the program to >> be already running by the time we call the API function, which isn't >> really what we want. >> >>> If that isn't possible, one fall-back might be to look up the >>> symbol table from the PC, and use the associated objfile arch. > > Here's the new version of the patch. It uses the objfile's gdbarch > and, if not available, python_gdbarch. > > diff --git a/gdb/python/python.c b/gdb/python/python.c > index d23325a..2dc2d41 100644 > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -703,6 +703,87 @@ gdbpy_solib_name (PyObject *self, PyObject *args) > return str_obj; > } > > +/* Returns 1 if the given PC may be inside a prologue, 0 if it > definitely isn't, Hi. A few comments. Broken patch. Cut-n-paste error or unhelpful mail program? > + and -1 if we have no debug info to use. */ > + > +static int > +pc_may_be_in_prologue (gdb_py_ulongest pc) > +{ > + int result = -1; > + struct symbol *function_symbol; > + struct symtab_and_line function_body_start_sal; > + > + function_symbol = find_pc_function(pc); > + > + if (function_symbol) gdb's coding style convention is to write function_symbol != NULL. > + { > + function_body_start_sal = find_function_start_sal (function_symbol, 1); > + > + result = pc < function_body_start_sal.pc; IWBN if the higher level API provided a routine rather than the python code having to hand-code this test. IOW, "pc_may_be_in_prologue" should live in gdb/*.c, not gdb/python/*.c. [As for which file, in gdb/*.c, symtab.c would be fine for now I think.] > + } > + > + return result; > +} > + Missing function comment for stack_is_destroyed. As a rule they all must have them. Plus the name "stack is destroyed" is confusing. This function is just a wrapper around gdbarch_in_function_epilogue_p so I'd just call it in_function_epilogue_p (or gdbpy_in_function_epilogue_p or some such). > +static int > +stack_is_destroyed (gdb_py_ulongest pc) > +{ > + int result; > + struct symtab *symtab = NULL; > + struct gdbarch *gdbarch = NULL; > + > + symtab = find_pc_symtab (pc); > + > + if ((symtab != NULL) && (symtab->objfile != NULL)) > + { > + gdbarch = get_objfile_arch (symtab->objfile); > + } Convention is to not use braces when the code occupies one line. > + > + if (gdbarch != NULL) > + { > + result = gdbarch_in_function_epilogue_p (gdbarch, pc); > + } > + else > + { > + result = gdbarch_in_function_epilogue_p (python_gdbarch, pc); > + } This code would be simpler if written as: if (gdbarch == NULL) gdbarch = python_gdbarch; result = gdbarch_function_in_epilogue_p (python_gdbarch); > + > + return result; > +} > + > +/* Returns True if a given PC may point to an address in which the stack frame > + may not be valid (either because it may not be set up yet or because it was > + destroyed, usually in a function's epilogue), False otherwise. */ > + > +static PyObject * > +gdbpy_stack_may_be_invalid (PyObject *self, PyObject *args) > +{ > + gdb_py_ulongest pc; > + PyObject *result = NULL; > + int pc_maybe_in_prologue; > + > + if (PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &pc)) > + { > + pc_maybe_in_prologue = pc_may_be_in_prologue (pc); > + > + if (pc_maybe_in_prologue != -1) > + { > + result = stack_is_destroyed (pc) || pc_maybe_in_prologue ? It'd be more efficient to avoid an unnecessary call to stack_is_destroyed by checking pc_maybe_in_prologue first. > + Py_True : Py_False; > + > + Py_INCREF (result); > + } > + else /* No debug info at that point. */ > + { > + PyErr_Format (PyExc_RuntimeError, > + _("There's no debug info for a function that\n" > + "could be enclosing the given PC.")); A newline in an error message feels odd. I'd remove it. > + } > + } > + > + return result; > +} > + > /* A Python function which is a wrapper for decode_line_1. */ > > static PyObject * > @@ -2000,6 +2081,15 @@ Return the selected inferior object." }, > { "inferiors", gdbpy_inferiors, METH_NOARGS, > "inferiors () -> (gdb.Inferior, ...).\n\ > Return a tuple containing all inferiors." }, > + > + > + { "stack_may_be_invalid", gdbpy_stack_may_be_invalid, METH_VARARGS, > + "stack_may_be_invalid (Long) -> Boolean.\n\ > +Returns True if a given PC may point to an address in which the stack frame\n\ > +may not be valid (either because it may not be set up yet or because it was\n\ > +destroyed, usually in a function's epilogue), False otherwise."}, The name "stack_may_be_invalid" is confusing. It's not that the stack is invalid, rather that locals in the stack frame are inaccessible. stack_frame_may_be_invalid? > + > + > {NULL, NULL, 0, NULL} > };
On 11/12/2014 05:06 PM, Doug Evans wrote: > > Plus the name "stack is destroyed" is confusing. > This function is just a wrapper around gdbarch_in_function_epilogue_p > so I'd just call it in_function_epilogue_p (or > gdbpy_in_function_epilogue_p or some such). Can we agree to rename the gdbarch hook instead? The function does _not_ return whether the PC is in the epilogue. https://sourceware.org/ml/gdb-patches/2014-10/msg00590.html Thanks, Pedro Alves
On Wed, Nov 12, 2014 at 2:06 PM, Doug Evans <dje@google.com> wrote: > Broken patch. Cut-n-paste error or unhelpful mail program? Probably the second one. Will fix it for the next version. >> + and -1 if we have no debug info to use. */ >> + >> +static int >> +pc_may_be_in_prologue (gdb_py_ulongest pc) >> +{ >> + int result = -1; >> + struct symbol *function_symbol; >> + struct symtab_and_line function_body_start_sal; >> + >> + function_symbol = find_pc_function(pc); >> + >> + if (function_symbol) > > gdb's coding style convention is to write function_symbol != NULL. Indeed, I must've missed that one! >> + { >> + function_body_start_sal = find_function_start_sal (function_symbol, 1); >> + >> + result = pc < function_body_start_sal.pc; > > IWBN if the higher level API provided a routine rather than the python > code having to hand-code this test. IOW, "pc_may_be_in_prologue" > should live in gdb/*.c, not gdb/python/*.c. > [As for which file, in gdb/*.c, symtab.c would be fine for now I think.] >> + } >> + >> + return result; >> +} >> + > > Missing function comment for stack_is_destroyed. > As a rule they all must have them. Will do. > Plus the name "stack is destroyed" is confusing. > This function is just a wrapper around gdbarch_in_function_epilogue_p > so I'd just call it in_function_epilogue_p (or > gdbpy_in_function_epilogue_p or some such). In the last thread ( https://www.sourceware.org/ml/gdb-patches/2014-10/msg00590.html ) we discussed that and Pedro pointed out that gdbarch_in_function_epilogue_p itself is misnamed, and we shouldn't carry that confusion to the Python API. >> +static int >> +stack_is_destroyed (gdb_py_ulongest pc) >> +{ >> + int result; >> + struct symtab *symtab = NULL; >> + struct gdbarch *gdbarch = NULL; >> + >> + symtab = find_pc_symtab (pc); >> + >> + if ((symtab != NULL) && (symtab->objfile != NULL)) >> + { >> + gdbarch = get_objfile_arch (symtab->objfile); >> + } > > Convention is to not use braces when the code occupies one line. As you wish. >> + >> + if (gdbarch != NULL) >> + { >> + result = gdbarch_in_function_epilogue_p (gdbarch, pc); >> + } >> + else >> + { >> + result = gdbarch_in_function_epilogue_p (python_gdbarch, pc); >> + } > > This code would be simpler if written as: > > if (gdbarch == NULL) > gdbarch = python_gdbarch; > > result = gdbarch_function_in_epilogue_p (python_gdbarch); >> + >> + return result; >> +} >> + >> +/* Returns True if a given PC may point to an address in which the stack frame >> + may not be valid (either because it may not be set up yet or because it was >> + destroyed, usually in a function's epilogue), False otherwise. */ >> + >> +static PyObject * >> +gdbpy_stack_may_be_invalid (PyObject *self, PyObject *args) >> +{ >> + gdb_py_ulongest pc; >> + PyObject *result = NULL; >> + int pc_maybe_in_prologue; >> + >> + if (PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &pc)) >> + { >> + pc_maybe_in_prologue = pc_may_be_in_prologue (pc); >> + >> + if (pc_maybe_in_prologue != -1) >> + { >> + result = stack_is_destroyed (pc) || pc_maybe_in_prologue ? > > It'd be more efficient to avoid an unnecessary call to > stack_is_destroyed by checking pc_maybe_in_prologue first. >> + Py_True : Py_False; >> + >> + Py_INCREF (result); >> + } >> + else /* No debug info at that point. */ >> + { >> + PyErr_Format (PyExc_RuntimeError, >> + _("There's no debug info for a function that\n" >> + "could be enclosing the given PC.")); > > A newline in an error message feels odd. > I'd remove it. >> + } >> + } >> + >> + return result; >> +} >> + >> /* A Python function which is a wrapper for decode_line_1. */ >> >> static PyObject * >> @@ -2000,6 +2081,15 @@ Return the selected inferior object." }, >> { "inferiors", gdbpy_inferiors, METH_NOARGS, >> "inferiors () -> (gdb.Inferior, ...).\n\ >> Return a tuple containing all inferiors." }, >> + >> + >> + { "stack_may_be_invalid", gdbpy_stack_may_be_invalid, METH_VARARGS, >> + "stack_may_be_invalid (Long) -> Boolean.\n\ >> +Returns True if a given PC may point to an address in which the stack frame\n\ >> +may not be valid (either because it may not be set up yet or because it was\n\ >> +destroyed, usually in a function's epilogue), False otherwise."}, > > The name "stack_may_be_invalid" is confusing. > It's not that the stack is invalid, rather that locals in the stack > frame are inaccessible. > stack_frame_may_be_invalid? Indeed, will fix those as well. Thanks a lot for the feedback!
On Wed, Nov 12, 2014 at 2:20 PM, Pedro Alves <palves@redhat.com> wrote: > On 11/12/2014 05:06 PM, Doug Evans wrote: >> >> Plus the name "stack is destroyed" is confusing. >> This function is just a wrapper around gdbarch_in_function_epilogue_p >> so I'd just call it in_function_epilogue_p (or >> gdbpy_in_function_epilogue_p or some such). > > Can we agree to rename the gdbarch hook instead? > The function does _not_ return whether the PC is in the epilogue. > > https://sourceware.org/ml/gdb-patches/2014-10/msg00590.html If everyone's ok with stack_frame_destroyed_p, I'll do the renaming myself and send it as a separate patch.
On Wed, Nov 12, 2014 at 9:20 AM, Pedro Alves <palves@redhat.com> wrote: > On 11/12/2014 05:06 PM, Doug Evans wrote: >> >> Plus the name "stack is destroyed" is confusing. >> This function is just a wrapper around gdbarch_in_function_epilogue_p >> so I'd just call it in_function_epilogue_p (or >> gdbpy_in_function_epilogue_p or some such). > > Can we agree to rename the gdbarch hook instead? > The function does _not_ return whether the PC is in the epilogue. > > https://sourceware.org/ml/gdb-patches/2014-10/msg00590.html "works for me"
diff --git a/gdb/python/python.c b/gdb/python/python.c index d23325a..2dc2d41 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -703,6 +703,87 @@ gdbpy_solib_name (PyObject *self, PyObject *args) return str_obj; } +/* Returns 1 if the given PC may be inside a prologue, 0 if it definitely isn't, + and -1 if we have no debug info to use. */ + +static int +pc_may_be_in_prologue (gdb_py_ulongest pc) +{ + int result = -1; + struct symbol *function_symbol; + struct symtab_and_line function_body_start_sal; + + function_symbol = find_pc_function(pc); + + if (function_symbol) + { + function_body_start_sal = find_function_start_sal (function_symbol, 1); + + result = pc < function_body_start_sal.pc; + } + + return result; +} + +static int +stack_is_destroyed (gdb_py_ulongest pc) +{ + int result; + struct symtab *symtab = NULL; + struct gdbarch *gdbarch = NULL; + + symtab = find_pc_symtab (pc); + + if ((symtab != NULL) && (symtab->objfile != NULL)) + { + gdbarch = get_objfile_arch (symtab->objfile); + } + + if (gdbarch != NULL) + { + result = gdbarch_in_function_epilogue_p (gdbarch, pc); + } + else + { + result = gdbarch_in_function_epilogue_p (python_gdbarch, pc); + } + + return result; +} + +/* Returns True if a given PC may point to an address in which the stack frame + may not be valid (either because it may not be set up yet or because it was + destroyed, usually in a function's epilogue), False otherwise. */ + +static PyObject * +gdbpy_stack_may_be_invalid (PyObject *self, PyObject *args) +{ + gdb_py_ulongest pc; + PyObject *result = NULL; + int pc_maybe_in_prologue; + + if (PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &pc)) + { + pc_maybe_in_prologue = pc_may_be_in_prologue (pc); + + if (pc_maybe_in_prologue != -1) + { + result = stack_is_destroyed (pc) || pc_maybe_in_prologue ? + Py_True : Py_False; + + Py_INCREF (result); + } + else /* No debug info at that point. */ + { + PyErr_Format (PyExc_RuntimeError, + _("There's no debug info for a function that\n" + "could be enclosing the given PC.")); + } + } + + return result; +} + /* A Python function which is a wrapper for decode_line_1. */