From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18880 invoked by alias); 11 Jan 2010 21:30:28 -0000 Received: (qmail 18855 invoked by uid 22791); 11 Jan 2010 21:30:26 -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:30:19 +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 o0BLUHfj026307 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 11 Jan 2010 16:30:17 -0500 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o0BLUGW9015542; Mon, 11 Jan 2010 16:30:16 -0500 Message-ID: <4B4B9867.5080805@redhat.com> Date: Mon, 11 Jan 2010 21:30:00 -0000 From: Phil Muldoon User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091209 Fedora/3.0-4.fc12 Lightning/1.0pre Thunderbird/3.0 MIME-Version: 1.0 To: tromey@redhat.com CC: gdb-patches ml Subject: Re: [patch][python] Implement Python lazy strings (PR 10705) References: <4B4746A7.90309@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2010-01/txt/msg00278.txt.bz2 On 01/11/2010 09:08 PM, Tom Tromey wrote: >>>>>> "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. We did. This case is slightly different case from original case we discussed in irc (the original was in the non-mi case). Anyway, if thevalue is not NULL (and in these cases it won't) it is printed via LA_PRINT_STRING (just a few lines further past this patch hunk) and the encoding happens at that point. > > 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. Ok. > 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. Ok. > 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. Ok. > Phil> + output = convert_value_from_python (string); > > Why do we need to do this? > Can't we just get the address directly? As a lazy string is a pointer, the code needs to actually find the type associated with the characters in the string. So later in the code: if (TYPE_CODE (value_type (output)) == TYPE_CODE_PTR) output = value_ind (output); I originally though about doing this in the creation of a lazy string. But if that value is processed through common_val_print (say in non a pretty printing case) the output for "foo" would be: "f" But if it is left as a pointer type it prints as: "foo" I'm not adverse to performing the value_ind action in the creation of the lazy string. I just thought it would be nice to allow lazy strings to print by common_val_print as well as our specialized case. > > 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. Ok. > > Variables like 'encoding' that are only used in one branch of the if > should just be declared in that branch. Ok. Mea culpa on the obvious mistakes ;) Cheers Phil