From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7999 invoked by alias); 12 Nov 2014 17:24:35 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 7988 invoked by uid 89); 12 Nov 2014 17:24:34 -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-qa0-f52.google.com Received: from mail-qa0-f52.google.com (HELO mail-qa0-f52.google.com) (209.85.216.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 12 Nov 2014 17:24:33 +0000 Received: by mail-qa0-f52.google.com with SMTP id u7so8565752qaz.25 for ; Wed, 12 Nov 2014 09:24:31 -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 :content-transfer-encoding; bh=ooRX95ESPWIvxD9wN7cWO0U2c3pgOeAF8drqXqiVuGU=; b=VcoZsI+LJXYarxcb1XSyXBjkvNTTuaFIAUM1+FzgMq1q63aD2/w8HN+uDkWOvW8RAe OA5SyNfh2sjqWExrbK5PZvDL2IE3SLOo97XevFGpT0re/MbCVKF+nthElYMgZhiEtnlq eJ92gfTzDcb571ToDJV2i40XMnKNN6wenrXBXBY2HubfteSaQTbEfL9v9K0FEuW8cPCT w1LJs5NvcidcAkvFoWUePDl7f46eTak9igNH8T3kWbhqSQ9sR1rsHYO5vgD2z7EL0Ufo 5vc98vA29nI8L+sQd0d36RvMlrxUdPTrgT0zQeQn/XGfvbkyRxlJwrHh4Zngv8R8mssa lfmA== X-Gm-Message-State: ALoCoQll2WP4EMecGLUNjuzxQU5fZSbqtumjNg1pfdkkrwZ6gGysVsHesJXnlZHWjCkNs0mUDk4C MIME-Version: 1.0 X-Received: by 10.224.25.80 with SMTP id y16mr45425772qab.15.1415813070963; Wed, 12 Nov 2014 09:24:30 -0800 (PST) Received: by 10.140.34.65 with HTTP; Wed, 12 Nov 2014 09:24:30 -0800 (PST) In-Reply-To: References: <201411071727.sA7HRNIQ007851@d03av02.boulder.ibm.com> Date: Wed, 12 Nov 2014 17:24:00 -0000 Message-ID: Subject: Re: [PING][RFC][PATCH v2] Python API: add gdb.stack_may_be_invalid From: Martin Galvan To: Doug Evans Cc: Ulrich Weigand , gdb-patches , Eli Zaretskii , Pedro Alves , Daniel Gutson Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2014-11/txt/msg00223.txt.bz2 On Wed, Nov 12, 2014 at 2:06 PM, Doug Evans 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 =3D -1; >> + struct symbol *function_symbol; >> + struct symtab_and_line function_body_start_sal; >> + >> + function_symbol =3D find_pc_function(pc); >> + >> + if (function_symbol) > > gdb's coding style convention is to write function_symbol !=3D NULL. Indeed, I must've missed that one! >> + { >> + function_body_start_sal =3D find_function_start_sal (function_sym= bol, 1); >> + >> + result =3D 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 =3D NULL; >> + struct gdbarch *gdbarch =3D NULL; >> + >> + symtab =3D find_pc_symtab (pc); >> + >> + if ((symtab !=3D NULL) && (symtab->objfile !=3D NULL)) >> + { >> + gdbarch =3D get_objfile_arch (symtab->objfile); >> + } > > Convention is to not use braces when the code occupies one line. As you wish. >> + >> + if (gdbarch !=3D NULL) >> + { >> + result =3D gdbarch_in_function_epilogue_p (gdbarch, pc); >> + } >> + else >> + { >> + result =3D gdbarch_in_function_epilogue_p (python_gdbarch, pc); >> + } > > This code would be simpler if written as: > > if (gdbarch =3D=3D NULL) > gdbarch =3D python_gdbarch; > > result =3D gdbarch_function_in_epilogue_p (python_gdbarch); >> + >> + return result; >> +} >> + >> +/* Returns True if a given PC may point to an address in which the stac= k 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 =3D NULL; >> + int pc_maybe_in_prologue; >> + >> + if (PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &pc)) >> + { >> + pc_maybe_in_prologue =3D pc_may_be_in_prologue (pc); >> + >> + if (pc_maybe_in_prologue !=3D -1) >> + { >> + result =3D 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 f= rame\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! --=20 Mart=C3=ADn Galv=C3=A1n Software Engineer Taller Technologies Argentina San Lorenzo 47, 3rd Floor, Office 5 C=C3=B3rdoba, Argentina Phone: 54 351 4217888 / +54 351 4218211