From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17746 invoked by alias); 12 Nov 2014 17:06:28 -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 17735 invoked by uid 89); 12 Nov 2014 17:06:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-vc0-f174.google.com Received: from mail-vc0-f174.google.com (HELO mail-vc0-f174.google.com) (209.85.220.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 12 Nov 2014 17:06:26 +0000 Received: by mail-vc0-f174.google.com with SMTP id la4so2669688vcb.33 for ; Wed, 12 Nov 2014 09:06:23 -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=hlDYcgJyBOicf1l35nQ9p+Rp4o295ooj7FpWcwSRgv4=; b=K8uCNsAPsdylWeSnI0Vb2WUPQDb7BC6t8UDP4JfU3/1pcDByNtoIq2TNtfQApZsxFx amWcdMjNw4N/yGG320YnxlvgKu+urrb1YVCP3v0d7PJRSccZ5mz9V0v1UKChvy642vzm rhohy3oDeOe7zmRr4sDMRJLbIovKR++QGVcWtfqouAIkPIW6GZlFUVmAYX+raa4+qH2k Sm2b3RmfK5Q8QfZHf6TO/38SqKeqbNhKvLDZfJZzKHpQnu+Iq6qMMje/1H5us5MWvYIB n9DlYxOE18W/63LgDFdoATEYDcL3Kz/ZgqDvlRVV2QVGdwc3xQhoDeW4ltWoUm2poo/9 0FTg== X-Gm-Message-State: ALoCoQkRsys7fAzf2xFeyC4k5h04qP/ufY4dfWbY//S8CON8x9N7gB1jGSZBFhgCI/BUcv2iiLzi MIME-Version: 1.0 X-Received: by 10.52.227.234 with SMTP id sd10mr15847715vdc.28.1415811983707; Wed, 12 Nov 2014 09:06:23 -0800 (PST) Received: by 10.52.114.101 with HTTP; Wed, 12 Nov 2014 09:06:23 -0800 (PST) In-Reply-To: References: <201411071727.sA7HRNIQ007851@d03av02.boulder.ibm.com> Date: Wed, 12 Nov 2014 17:06:00 -0000 Message-ID: Subject: Re: [PING][RFC][PATCH v2] Python API: add gdb.stack_may_be_invalid From: Doug Evans To: Martin Galvan Cc: Ulrich Weigand , gdb-patches , Eli Zaretskii , Pedro Alves , Daniel Gutson Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg00221.txt.bz2 On Wed, Nov 12, 2014 at 7:55 AM, Martin Galvan wrote: > On Fri, Nov 7, 2014 at 2:36 PM, Martin Galvan > wrote: >> On Fri, Nov 7, 2014 at 2:27 PM, Ulrich Weigand 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} > };