From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1522 invoked by alias); 11 Jan 2010 21:08:09 -0000 Received: (qmail 1501 invoked by uid 22791); 11 Jan 2010 21:08:08 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 11 Jan 2010 21:08:03 +0000 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o0BL82xr011195 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 11 Jan 2010 16:08:02 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o0BL81Ax009827; Mon, 11 Jan 2010 16:08:02 -0500 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o0BL80ls002910; Mon, 11 Jan 2010 16:08:01 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 4D64B378187; Mon, 11 Jan 2010 14:08:00 -0700 (MST) From: Tom Tromey To: Phil Muldoon Cc: gdb-patches ml Subject: Re: [patch][python] Implement Python lazy strings (PR 10705) References: <4B4746A7.90309@redhat.com> Reply-To: tromey@redhat.com Date: Mon, 11 Jan 2010 21:08:00 -0000 In-Reply-To: <4B4746A7.90309@redhat.com> (Phil Muldoon's message of "Fri, 08 Jan 2010 14:52:23 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2010-01/txt/msg00274.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Phil> Index: gdb/varobj.c [...] Phil> + if (is_lazy_string (output)) Phil> + thevalue = extract_lazy_string (output, &type, Phil> + &len, &encoding); Phil> + else Phil> { Phil> - char *s = PyString_AsString (py_str); Phil> - len = PyString_Size (py_str); Phil> - thevalue = xmemdup (s, len + 1, len + 1); Phil> - Py_DECREF (py_str); Phil> + PyObject *py_str Phil> + = python_string_to_target_python_string (output); Phil> + if (py_str) Phil> + { Phil> + char *s = PyString_AsString (py_str); Phil> + len = PyString_Size (py_str); Phil> + thevalue = xmemdup (s, len + 1, len + 1); Phil> + type = builtin_type (gdbarch)->builtin_char; Phil> + Py_DECREF (py_str); Phil> + } Phil> } Phil> Py_DECREF (output); Phil> } Phil> if (thevalue && !string_print) Phil> { Phil> do_cleanups (back_to); Phil> + xfree (encoding); Phil> return thevalue; This is wrong in the lazy string case, because you are returning raw bytes, but the user expects them to be interpreted according to the encoding. We discussed this on irc and I thought the result was that we agreed that in this case we would pretend that a "string" hint was given. One way to do this would be to set "string_print = 1" in the lazy case. The documentation for the "string" hint should mention this. Phil> +PyObject * Phil> +gdbpy_create_lazy_string_object (CORE_ADDR address, long length, Phil> + const char *encoding, struct type *type) Phil> +{ [...] Phil> + if (!str_obj) Phil> + return NULL; Indentation looks wrong on the second line. Phil> +/* Determine whether the printer object pointed to by OBJ is a Phil> + Python lazy string. */ Phil> +int Phil> +is_lazy_string (PyObject *result) Phil> +{ Phil> + return PyObject_TypeCheck (result, &lazy_string_object_type); Phil> +} Why here and not in py-lazy-string.c? Then lazy_string_object_type could be static. Phil> +/* Extract and return the actual string from the lazy string object Phil> + STRING. Additionally, the string type is written to *STR_TYPE, the Phil> + string length is written to *LENGTH, and the string encoding is Phil> + written to *ENCODING. On error, NULL is returned. The caller is Phil> + responsible for freeing the returned buffer. */ Phil> +gdb_byte * Phil> +extract_lazy_string (PyObject *string, struct type **str_type, Phil> + long *length, char **encoding) Likewise. Phil> + output = convert_value_from_python (string); Why do we need to do this? Can't we just get the address directly? Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { Phil> + Extra blank line. Phil> + if (except.reason < 0) Phil> + { Phil> + Likewise. Phil> +} Phil> + Phil> + Phil> + Likewise. Phil> + int is_lazy = 0; Phil> + Phil> + is_lazy = is_lazy_string (py_str); There's no need to initialize is_lazy to 0 if you then immediately assign to it. Phil> + gdb_byte *output = NULL; Phil> + struct type *type; Phil> + long length; Phil> + char *encoding = NULL; Phil> + Phil> + if (is_lazy_string (py_v)) Phil> + { Phil> + output = extract_lazy_string (py_v, &type, &length, &encoding); Phil> + if (!output) Phil> + gdbpy_print_stack (); Phil> + LA_PRINT_STRING (stream, type, output, length, encoding, Phil> + 0, options); Phil> + xfree (encoding); Phil> + xfree (output); Variables like 'encoding' that are only used in one branch of the if should just be declared in that branch. Tom