This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH v5 8/9] python: Add tests for record Python bindings
- From: "Wiederhake, Tim" <tim dot wiederhake at intel dot com>
- To: Doug Evans <xdje42 at gmail dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, "Metzger, Markus T" <markus dot t dot metzger at intel dot com>, "palves at redhat dot com" <palves at redhat dot com>
- Date: Mon, 13 Feb 2017 12:38:37 +0000
- Subject: RE: [PATCH v5 8/9] python: Add tests for record Python bindings
- Authentication-results: sourceware.org; auth=none
- References: <1485527996-32506-1-git-send-email-tim.wiederhake@intel.com> <1485527996-32506-9-git-send-email-tim.wiederhake@intel.com> <m3poimrboa.fsf@sspiff.org>
> Hi. A few issues noted below.
Hi Doug, thanks for reviewing. See below for comments.
> > +/* Implementation of BtraceInstruction.sal [gdb.Symtab_and_line].
> > + Return the symbol associated with this instruction. */
> s/symbol/SAL/
Changed locally.
> > + return PyMemoryView_FromObject (object); }
> I looked at py-inferior.c:infpy_read_memory.
> Not sure how much we want to be consistent here, do we want to wrap the
> result here in a "membuf"?
> I can believe it's overkill for the task at hand, just thought I'd mention it.
> I'm assuming there are no python2-vs-3 issues here.
To use "membuf", I'd have to factor it out from py-inferior.c and adapt it to support read-only buffers.
Memoryview is part of the Python standard library in Python 2.7 as well as Python 3 and seemed like the right way to go.
> > + strfile = mem_fileopen ();
> > + gdb_print_insn (target_gdbarch (), insn->pc, strfile, NULL);
> I don't know if any of the btrace_* functions an throw a gdb error, but
> gdb_print_insn can.
> So at least that needs to be wrapped in a TRY/CATCH.
>
> Suggest looking at the c++ support that archpy_disassemble uses.
> mem_fileopen is gone now.
Changed locally.
> > +static PyObject *
> > +btpy_list_count (PyObject *self, PyObject *value) {
> > + const LONGEST index = btpy_list_position (self, value);
> > +
> > + if (index < 0)
> > + return PyInt_FromLong (0);
> > + else
> > + return PyInt_FromLong (1);
>
> It's not obvious from a simple reading why "count" is 0 or 1.
> Suggest adding a comment.
Changed locally.
> > + TRY
> > + {
> > + struct btrace_insn_iterator iter;
> > + char buffer[256] = { '\0' };
> Buffer is unused.
Changed locally.
> > +struct PyGetSetDef btpy_insn_getset[] = {
> "{" goes on line by itself
This is handled very inconsistently. For Example, py-arch.c and py-frame.c (and others) have the opening curly brace on the same line whereas py-type.c puts it on the next line. Locally changed as requested.
Tim
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928