From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2096 invoked by alias); 4 Jul 2011 06:04:11 -0000 Received: (qmail 2081 invoked by uid 22791); 4 Jul 2011 06:04:10 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,T_TO_NO_BRKTS_FREEMAIL X-Spam-Check-By: sourceware.org Received: from mail-gy0-f169.google.com (HELO mail-gy0-f169.google.com) (209.85.160.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 04 Jul 2011 06:03:51 +0000 Received: by gyg13 with SMTP id 13so2291303gyg.0 for ; Sun, 03 Jul 2011 23:03:51 -0700 (PDT) MIME-Version: 1.0 Received: by 10.236.116.202 with SMTP id g50mr6890012yhh.505.1309759431200; Sun, 03 Jul 2011 23:03:51 -0700 (PDT) Received: by 10.236.95.167 with HTTP; Sun, 3 Jul 2011 23:03:51 -0700 (PDT) In-Reply-To: References: Date: Mon, 04 Jul 2011 06:36:00 -0000 Message-ID: Subject: Re: [patch] [python] find_line_pc_range From: Matt Rice To: pmuldoon@redhat.com Cc: gdb-patches@sourceware.org Content-Type: multipart/mixed; boundary=20cf3036388da3511a04a7382125 X-IsSubscribed: yes 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 X-SW-Source: 2011-07/txt/msg00081.txt.bz2 --20cf3036388da3511a04a7382125 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Content-length: 3175 On Sun, Jul 3, 2011 at 9:18 AM, Phil Muldoon wrote: > Matt Rice writes: > >> don't know a whole much about python but, >> would it be better to return None on error, instead of a tuple containin= g Nones? > > Thanks. I would return either an exception or None depending on the > situational context of the failure. =A0If a None type scenario is expecte= d, > and normal then it is okay to return that, given that the Python > scripter would understand the reason for that. =A0So documentation is key > here. =A0I think find_line_pc_range is just a utility function, > which can very well not find the line range. =A0So I would replicate that > functionality in your API, except I would return a single Py_None over a > tuple. > > >> +salpy_find_line_pc_range (PyObject *self, PyObject *args) >> +{ >> + =A0struct symtab_and_line *sal =3D NULL; >> + =A0CORE_ADDR start_pc, end_pc; >> + =A0PyObject *start; >> + =A0PyObject *end; >> + =A0PyObject *ret_tuple; >> + >> + =A0SALPY_REQUIRE_VALID (self, sal); >> + >> + =A0ret_tuple =3D PyTuple_New (2); > > This call can fail and return NULL, so in this case you need to return > NULL to propagate the failure. > >> + =A0if (find_line_pc_range (*sal, &start_pc, &end_pc)) >> + =A0 =A0{ >> + >> + =A0 =A0 =A0start =3D gdb_py_long_from_longest (start_pc); >> + =A0 =A0 =A0end =3D gdb_py_long_from_longest (end_pc); >> + =A0 =A0 =A0PyTuple_SET_ITEM (ret_tuple, 0, start); >> + =A0 =A0 =A0PyTuple_SET_ITEM (ret_tuple, 1, end); >> + =A0 =A0} > > start and end could be local to this block I think. > > Also gdb_py_long_from_longest is macro that points to Py > Long_FromUnsignedLongLong. =A0This can also return NULL which indicates a > failure, so similar to above, you need to check the return and return > NULL if one of them fails. =A0If one of them does return NULL, you also > need to appropriately clean-up previous references that were created in > Python. =A0We normally do this by creating a local error: goto. > > A minor nit, SET_ITEM does not do any error checking, while SetItem > does. =A0As this is a new tuple, then it is ok to use it. =A0But normally > (and personally, in new tuples) I always use the error checking > equivalent. > > > + =A0else > + =A0 =A0{ > + =A0 =A0 =A0PyTuple_SET_ITEM (ret_tuple, 0, Py_None); > + =A0 =A0 =A0PyTuple_SET_ITEM (ret_tuple, 1, Py_None); > + =A0 =A0} > + > > In this case I would just return Py_None (see first paragraph). =A0It > seems a little redundant to return a tuple with Py_None for both > elements, and this will always be the case on a lookup failure with > find_line_pc_range. Make sure you incref Py_None before returning it, > also. > > Also, this patch needs tests. Thanks, attached is an updated patch that also includes tests. 2011-07-03 Matt Rice * python/py-symtab.c: Populate sal_object_methods. (salpy_find_line_pc_range): New function. 2011-07-03 Matt Rice * gdb.texinfo (Symbol Tables In Python): Add find_line_pc_range met= hod. 2011-07-03 Matt Rice * gdb.python/py-symtab.exp: New Tests for find_line_pc_range. --20cf3036388da3511a04a7382125 Content-Type: application/octet-stream; name="foo.diff" Content-Disposition: attachment; filename="foo.diff" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gpp0u9250 Content-length: 4547 ZGlmZiAtLWdpdCBhL2dkYi9kb2MvZ2RiLnRleGluZm8gYi9nZGIvZG9jL2dk Yi50ZXhpbmZvCmluZGV4IGRiYWYzMGUuLmM2MzdiYTIgMTAwNjQ0Ci0tLSBh L2dkYi9kb2MvZ2RiLnRleGluZm8KKysrIGIvZ2RiL2RvYy9nZGIudGV4aW5m bwpAQCAtMjMyOTMsNiArMjMyOTMsMTEgQEAgZXhpc3QgaW4gQHZhbHVle0dE Qk59IGFueSBsb25nZXIuICBBbGwgb3RoZXIKIEBjb2Rle2dkYi5TeW10YWJf YW5kX2xpbmV9IG1ldGhvZHMgd2lsbCB0aHJvdyBhbiBleGNlcHRpb24gaWYg aXQgaXMKIGludmFsaWQgYXQgdGhlIHRpbWUgdGhlIG1ldGhvZCBpcyBjYWxs ZWQuCiBAZW5kIGRlZm1ldGhvZAorCitAZGVmbWV0aG9kIFN5bXRhYl9hbmRf bGluZSBmaW5kX2xpbmVfcGNfcmFuZ2UKK0lmIGZvdW5kIHJldHVybnMgYSBA Y29kZXtUdXBsZX0gY29udGFpbmluZyB0aGUgc3RhcnQgYW5kIGVuZCBwcm9n cmFtIGNvdW50ZXIKK2FkZHJlc3NlcyBmb3IgdGhlIGxpbmUgYXR0cmlidXRl LiAgT3RoZXJ3aXNlIHJldHVybnMgQGNvZGV7Tm9uZX0uCitAZW5kIGRlZm1l dGhvZAogQGVuZCB0YWJsZQogCiBBIEBjb2Rle2dkYi5TeW10YWJ9IG9iamVj dCBoYXMgdGhlIGZvbGxvd2luZyBhdHRyaWJ1dGVzOgpkaWZmIC0tZ2l0IGEv Z2RiL3B5dGhvbi9weS1zeW10YWIuYyBiL2dkYi9weXRob24vcHktc3ltdGFi LmMKaW5kZXggMTA3Y2RlYy4uMjE4MTFkMSAxMDA2NDQKLS0tIGEvZ2RiL3B5 dGhvbi9weS1zeW10YWIuYworKysgYi9nZGIvcHl0aG9uL3B5LXN5bXRhYi5j CkBAIC0yMTUsNiArMjE1LDUzIEBAIHNhbHB5X2dldF9saW5lIChQeU9iamVj dCAqc2VsZiwgdm9pZCAqY2xvc3VyZSkKIH0KIAogc3RhdGljIFB5T2JqZWN0 ICoKK3NhbHB5X2ZpbmRfbGluZV9wY19yYW5nZSAoUHlPYmplY3QgKnNlbGYs IFB5T2JqZWN0ICphcmdzKQoreworICBzdHJ1Y3Qgc3ltdGFiX2FuZF9saW5l ICpzYWwgPSBOVUxMOworICBDT1JFX0FERFIgc3RhcnRfcGMsIGVuZF9wYzsK KworICBTQUxQWV9SRVFVSVJFX1ZBTElEIChzZWxmLCBzYWwpOworCisgIGlm IChmaW5kX2xpbmVfcGNfcmFuZ2UgKCpzYWwsICZzdGFydF9wYywgJmVuZF9w YykpCisgICAgeworICAgICAgUHlPYmplY3QgKnJldF90dXBsZTsKKyAgICAg IFB5T2JqZWN0ICpzdGFydDsKKyAgICAgIFB5T2JqZWN0ICplbmQ7CisKKyAg ICAgIHJldF90dXBsZSA9IFB5VHVwbGVfTmV3ICgyKTsKKyAgICAgIGlmICgh cmV0X3R1cGxlKQorICAgICAgICByZXR1cm4gUHlfTm9uZTsKKworICAgICAg c3RhcnQgPSBnZGJfcHlfbG9uZ19mcm9tX3Vsb25nZXN0IChzdGFydF9wYyk7 CisgICAgICBpZiAoIXN0YXJ0KQorCWdvdG8gZmFpbF9yZXQ7CisgICAgICBp ZiAoUHlUdXBsZV9TZXRJdGVtIChyZXRfdHVwbGUsIDAsIHN0YXJ0KSAhPSAw KQorCXsKKwkgIFB5X1hERUNSRUYgKHN0YXJ0KTsKKwkgIGdvdG8gZmFpbF9y ZXQ7CisJfQorCisgICAgICBlbmQgPSBnZGJfcHlfbG9uZ19mcm9tX3Vsb25n ZXN0IChlbmRfcGMpOworICAgICAgaWYgKCFlbmQpCisJZ290byBmYWlsX3Jl dDsKKyAgICAgIGlmIChQeVR1cGxlX1NldEl0ZW0gKHJldF90dXBsZSwgMSwg ZW5kKSAhPSAwKQorCXsKKwkgIFB5X1hERUNSRUYgKGVuZCk7CisJICBnb3Rv IGZhaWxfcmV0OworCX0KKworICAgICAgcmV0dXJuIHJldF90dXBsZTsKKwor ICAgICAgZmFpbF9yZXQ6CisJUHlfWERFQ1JFRiAocmV0X3R1cGxlKTsKKwly ZXR1cm4gUHlfTm9uZTsKKyAgICB9CisKKyAgcmV0dXJuIFB5X05vbmU7CisK K30KKworc3RhdGljIFB5T2JqZWN0ICoKIHNhbHB5X2dldF9zeW10YWIgKFB5 T2JqZWN0ICpzZWxmLCB2b2lkICpjbG9zdXJlKQogewogICBzdHJ1Y3Qgc3lt dGFiX2FuZF9saW5lICpzYWw7CkBAIC01MjYsNiArNTczLDggQEAgc3RhdGlj IFB5TWV0aG9kRGVmIHNhbF9vYmplY3RfbWV0aG9kc1tdID0gewogICB7ICJp c192YWxpZCIsIHNhbHB5X2lzX3ZhbGlkLCBNRVRIX05PQVJHUywKICAgICAi aXNfdmFsaWQgKCkgLT4gQm9vbGVhbi5cblwKIFJldHVybiB0cnVlIGlmIHRo aXMgc3ltYm9sIHRhYmxlIGFuZCBsaW5lIGlzIHZhbGlkLCBmYWxzZSBpZiBu b3QuIiB9LAorICB7ICJmaW5kX2xpbmVfcGNfcmFuZ2UiLCBzYWxweV9maW5k X2xpbmVfcGNfcmFuZ2UsIE1FVEhfTk9BUkdTLAorICAgICJSZXR1cm4gYSB0 dXBsZSBvZiBzdGFydCBhbmQgZW5kIHJhbmdlcyBmb3IgdGhlIHN5bWJvbCB0 YWJsZS4iIH0sCiAgIHtOVUxMfSAgLyogU2VudGluZWwgKi8KIH07CiAKZGlm ZiAtLWdpdCBhL2dkYi90ZXN0c3VpdGUvZ2RiLnB5dGhvbi9weS1zeW10YWIu ZXhwIGIvZ2RiL3Rlc3RzdWl0ZS9nZGIucHl0aG9uL3B5LXN5bXRhYi5leHAK aW5kZXggYzUyZjVlZi4uYjZmMzQyMyAxMDA2NDQKLS0tIGEvZ2RiL3Rlc3Rz dWl0ZS9nZGIucHl0aG9uL3B5LXN5bXRhYi5leHAKKysrIGIvZ2RiL3Rlc3Rz dWl0ZS9nZGIucHl0aG9uL3B5LXN5bXRhYi5leHAKQEAgLTU4LDYgKzU4LDE0 IEBAIGdkYl90ZXN0ICJweXRob24gcHJpbnQgc2FsLnN5bXRhYiIgImdkYi90 ZXN0c3VpdGUvZ2RiLnB5dGhvbi9weS1zeW1ib2wuYy4qIiAiVGVzCiBnZGJf dGVzdCAicHl0aG9uIHByaW50IHNhbC5wYyIgIiR7ZGVjaW1hbH0iICJUZXN0 IHNhbC5wYyIKIGdkYl90ZXN0ICJweXRob24gcHJpbnQgc2FsLmxpbmUiICI0 MiIgIlRlc3Qgc2FsLmxpbmUiCiBnZGJfdGVzdCAicHl0aG9uIHByaW50IHNh bC5pc192YWxpZCgpIiAiVHJ1ZSIgIlRlc3Qgc2FsLmlzX3ZhbGlkIgorZ2Ri X3Rlc3QgInB5dGhvbiBwcmludCBzYWwucGMgPT0gc2FsLmZpbmRfbGluZV9w Y19yYW5nZSgpXFswXF0iICJUcnVlIiAiVGVzdCBzYWwuZmluZF9saW5lX3Bj X3JhbmdlKCkgMSIKK2dkYl90ZXN0ICJweXRob24gcHJpbnQgc2FsLmZpbmRf bGluZV9wY19yYW5nZSgpXFsxXF0iICIke2RlY2ltYWx9IiAiVGVzdCBzYWwu ZmluZF9saW5lX3BjX3JhbmdlKCkgMiIKKworIyBUZXN0IHNhbCBmaW5kX2xp bmVfcGNfcmFuZ2UgZXJyb3JzLgorZ2RiX3Rlc3QgInB5dGhvbiBiYWRfbGlu ZV9zYWwgPSBnZGIuZGVjb2RlX2xpbmUoXCJweS1zeW1ib2wuYzo0MDRcIilc WzFcXVxbMFxdIiAiIiBcCisidGVzdCBkZWNvZGVfbGluZSBweXRob24uYzo0 MDQiCitnZGJfdGVzdCAicHl0aG9uIHByaW50IGJhZF9saW5lX3NhbC5maW5k X2xpbmVfcGNfcmFuZ2UoKSA9PSBOb25lIiAiVHJ1ZSIgXAorImZpbmRfbGlu ZV9wY19yYW5nZSBmb3Igbm9uLWV4aXN0ZW50IGxpbmUiCiAKICMgVGVzdCBz eW1ib2wgdGFibGUuCiBnZGJfdGVzdCAicHl0aG9uIHByaW50IHN5bXRhYi5m aWxlbmFtZSIgInRlc3RzdWl0ZS9nZGIucHl0aG9uL3B5LXN5bWJvbC5jLioi ICJUZXN0IHN5bXRhYi5maWxlbmFtZSIK --20cf3036388da3511a04a7382125--