From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 459 invoked by alias); 6 Aug 2012 11:12:12 -0000 Received: (qmail 443 invoked by uid 22791); 6 Aug 2012 11:12:10 -0000 X-SWARE-Spam-Status: No, hits=-4.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-bk0-f41.google.com (HELO mail-bk0-f41.google.com) (209.85.214.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 06 Aug 2012 11:11:55 +0000 Received: by bkcjc3 with SMTP id jc3so1013240bkc.0 for ; Mon, 06 Aug 2012 04:11:54 -0700 (PDT) Received: by 10.204.148.78 with SMTP id o14mr3883447bkv.38.1344251514221; Mon, 06 Aug 2012 04:11:54 -0700 (PDT) Received: from [10.0.0.3] (d83-191-103-145.cust.tele2.at. [83.191.103.145]) by mx.google.com with ESMTPS id fu8sm7226834bkc.5.2012.08.06.04.11.52 (version=SSLv3 cipher=OTHER); Mon, 06 Aug 2012 04:11:53 -0700 (PDT) Message-ID: <501FA677.6030404@googlemail.com> Date: Mon, 06 Aug 2012 11:12:00 -0000 From: Oliver Buchtala User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120615 Thunderbird/13.0.1 MIME-Version: 1.0 To: Phil Muldoon CC: gdb-patches@sourceware.org Subject: Re: patch for #14363 References: <501AA900.8020205@googlemail.com> <501F9FA1.9060203@redhat.com> In-Reply-To: <501F9FA1.9060203@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: 2012-08/txt/msg00162.txt.bz2 On 06.08.2012 12:42, Phil Muldoon wrote: > On 08/02/2012 05:21 PM, Oliver Buchtala wrote: >> From 05a921d423793db93dfead59c9fe12d342f55d75 Mon Sep 17 00:00:00 2001 >> From: Oliver Buchtala >> Date: Wed, 1 Aug 2012 21:59:43 +0200 >> Subject: [PATCH] Adapt py-prettyprint to inform pretty-printer about current >> recursion level. >> >> --- >> gdb/python/py-prettyprint.c | 3 +++ >> gdb/python/python-internal.h | 1 + >> gdb/python/python.c | 2 ++ >> 3 files changed, 6 insertions(+) > Thanks. > > Needs a GNU style ChangeLog, testcase and documentation > additions. > > I am having trouble understanding what you are doing with "recurse" > attribute. From valprint.c, in the GDB sources (which calls > apply_val_pretty_printer, among others): > > "RECURSE indicates the amount of indentation to supply before > continuation lines; this amount is roughly twice the value of > RECURSE." > > So I am not sure that this is the value you want? Anyway, I think you > want the number of recursions for that printer, not the recurse value > being supplied to apply_val_pretty_printer. A testcase would help > here to understand your intention. > >> diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c >> index 86d4f2c..d5c6b66 100644 >> --- a/gdb/python/py-prettyprint.c >> +++ b/gdb/python/py-prettyprint.c >> @@ -476,6 +476,9 @@ print_children (PyObject *printer, const char *hint, >> if (! PyObject_HasAttr (printer, gdbpy_children_cst)) >> return; >> >> + if ( PyObject_HasAttr (printer, gdbpy_level_cst)) >> + PyObject_SetAttr(printer, gdbpy_level_cst, PyInt_FromLong((long) recurse)); >> + > There are a few issues with this patch hunk: > > * It is not clear in the documentation, but PyInt_FromLong can raise a > PyErr_NoMemory exception, so you need to NULL check the return and > dispatch the exception appropriately. > > * This leaks a reference from PyInt_FromLong. PyObject_SetAttr > increments the reference count of the right side of "=" Python > equivalent (IE foo.bar = baz), which in this case is the integer. So > the transient reference from PyInt_FromLong will last forever. You > you need to assign the results of PyInt_FromLong to a variable and > call Py_DECREF on it after the "PyObject_SetAttr" call. > > * PyObject_SetAttr can fail, so the code needs to deal with this > contingency. > > * Why PyInt_FromLong over PyLong_FromLong? > > Beyond the patch fixes, is it not possible to count the recursion > level by some internal bookkeeping in the printer? I have no problem > with including the attribute if it is helpful, or clearer to solve the > problem, but I am curious if you have tried the alternative, if > possible? > > Cheers, > > Phil Thanks, I see there are lots of deficiencies with my patch. Sorry for that. You are right in all your points. I was not aware that recurse == indentation. (misnaming? ;) ) For the second proposal, I need a set data structure for longs. Do you have such a data structure in gdb somewhere? Or which should I use? Then I would offer a prototype implementation. Regards, Oliver