This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch][python] Implement Python lazy strings (PR 10705)
- From: Phil Muldoon <pmuldoon at redhat dot com>
- To: tromey at redhat dot com
- Cc: gdb-patches ml <gdb-patches at sourceware dot org>
- Date: Mon, 11 Jan 2010 21:30:15 +0000
- Subject: Re: [patch][python] Implement Python lazy strings (PR 10705)
- References: <4B4746A7.90309@redhat.com> <m3y6k4nydr.fsf@fleche.redhat.com>
On 01/11/2010 09:08 PM, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> 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