This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH v6 9/9] Add documentation for new record Python bindings.


Hi Yao!

I believe that there is a misunderstanding.  Let me pick up the idea of giving
concrete examples.  With the current implementation, a simple use case would
look like this:

(gdb) record btrace
[RECORD SOME PROGRAM FLOW]
(gdb) python-interactive
>>> recorded_instructions = gdb.current_recording().instruction_history
>>> for instruction in recorded_instructions:
...   print(i.number, hex(i.pc), i.sal)
...
[OUTPUT]

For the sake of the argument, let's assume that "number", "pc" and "sal" are not
unique to btrace and that support for "full" is implemented in the way that I
suggested: "FullInstruction" has some functions / data members that happen to
have the same names as their "BtraceInstruction" counterparts but these python
classes are not related in any way in terms of class inheritance. The user now
wants to record with method "full" and "instruction_history" returns not a list
of BtraceInstruction objects but a list of FullInstruction objects; the input
looks like this:

(gdb) record full
[RECORD SOME PROGRAM FLOW]
(gdb) python-interactive
>>> recorded_instructions = gdb.current_recording().instruction_history
>>> for instruction in recorded_instructions:
...   print(i.number, hex(i.pc), i.sal)
...
[OUTPUT]

It is exactly the same. Now if we were to have some Python instruction base
class, let's call it "RecordInstruction", and two Python sub classes,
"BtraceInstruction" and "FullInstruction", the example would still look exactly
like this:

(gdb) record [full or btrace]
[RECORD SOME PROGRAM FLOW]
(gdb) python-interactive
>>> recorded_instructions = gdb.current_recording().instruction_history
>>> for instruction in recorded_instructions:
...   print(i.number, hex(i.pc), i.sal)
...
[OUTPUT]

The user has no benefit from the existance of a python base class. They never
receive an instance of this base class to see what the common interface is. The
pythonic way -- as far as I understand it -- is to rely on the existance of
functions / data members of the objects, not on the actual type of the object.
That is what I meant with "duck typing". Both BtraceInstruction and
FullInstruction behave in the same way for a couple of functions / data members,
they "quack like a duck" and therefore "are a duck", no matter that they are
instances of two unrelated classes.

What exactly forms this common interface has yet to defined and codified in the
documentation. But this is in my opinion the beauty of this approach: All
functions / data members of BtraceInstruction that are not part of this common
interface automatically are "just there" and "additional functionality".

The approach of having a common base class /would/ have some benefit, if all
three of "RecordInstruction", "BtraceInstruction" and "FullInstruction" were
native Python classes: Less code in BtraceInstruction and FullInstruction. But
BtraceInstruction and eventually FullInstruction are "Built-In"s from Python's
perspective. That is, not real Python classes but some C code that interfaces
with the Python interpreter.

Let's assume that "number", "pc" and "sal" were to be the common interface and
therefore data members of "RecordInstruction". The C code for this Python class
now needs to know which recording is currently active, since there is no

current_target.to_get_pc_for_recorded_instruction_by_number(struct target_ops*,
							unsigned int);

So the C code for the common "RecordInstruction.pc" would look something like
this:

switch (current_recording_method)
  {
    case METHOD_BRACE:
	/* BTRACE specific code to retrieve PC for a given recorded instruction */
	struct btrace_insn_iterator iter;
	find_insn_by_number(&iter, number_of_the_recorded_instruction);
	struct btrace_insn* insn = btrace_insn_get(&iter);
	return insn->pc;
    case METHOD_FULL:
	/* FULL specific code to retrieve PC for a given recorded instruction */
	....
    default:
	/* throw some python exception */
  }

To add more recording related functions to target_ops is probably not the best
way to go, since we still need some shim to turn the returned C structs into
Python objects and we impose some requirements on the recording targets like
"requires random access to the recorded instruction" and so on. Additionally,
we would still require custom code for a recording method, if that recording
method can provide additional information that we want to make accessible to
the user.

From my point of view, there is no benefit for the user from the common base
class approach. Both cases exhibit the same functionality but the one with the
common base class adds lots of complexity and added code to maintain.

So, no "if (method = ...)" in python code; the manual tells you how write
(python) code that works regardless of the recording method in a yet to be
written and to be decided upon paragraph about the "common interface"; and the
python API itself is actually not at all btrace specific, as one could argue
that all implemented functions and data members are currently just "added
functionality for the btrace case".

I hope I could clear things up and thanks for reading through this,

Tim


> -----Original Message-----
> From: Yao Qi [mailto:qiyaoltc@gmail.com]
> Sent: Tuesday, March 7, 2017 12:54 PM
> To: Wiederhake, Tim <tim.wiederhake@intel.com>
> Cc: gdb-patches@sourceware.org; Metzger, Markus T
> <markus.t.metzger@intel.com>; palves@redhat.com; xdje42@gmail.com
> Subject: Re: [PATCH v6 9/9] Add documentation for new record Python
> bindings.
> 
> "Wiederhake, Tim" <tim.wiederhake@intel.com> writes:
> 
> >> I read them again this morning, and have a design question in
> >> general, why are they specific to btrace?
> >> (...)
> >> Can add a base class like RecordInstruction, which have some basic
> >> attributes for all record methods, and BtraceInstruction extends it
> >> to add more btrace specific attributes.
> >
> > Unfortunately, the interfaces to the "full" and "btrace" recording
> > differ quite much. So even for "common" attributes, the code for the
> > base class instruction and function segment objects would have to "if
> > (method == ...)" through all recording methods, cluttering it, making
> > it harder to maintain and potentially
> 
> Do you have a concrete example?  If you have to "if (method == ...)" in
> python code (outside of gdb), there is something wrong on design in python
> code.  I believe there are some design patterns for this kind of issue "if
> (method == ...)".
> 
> > cause trouble if we introduce another recording format in the future
> > when functions in the base class suddenly are not implemented for all
> > methods anymore.
> >
> > My idea was to make use of Python's philosophy of duck-typing. Having
> > specific (C wrapper) objects for each recording method solves the
> > problem of how to obtain the requested data from the recording "back
> > end". If we name respective functions and attributes the same in these
> > classes, we "quack like a duck" and create the same common interface for
> the user from Python's perspective.
> >
> 
> but you can't have a common interface because the python api is btrace
> specific.  Suppose we have gdb.FullInstruction for "full", what attributes do
> we have? .number and .pc maybe?  With the python api design like this, how
> can I write a python code to trace data regardless of which method is being
> used?  Ideal design in my mind is like this (I hope concrete example like this
> helps discussion)
> 
> class RecordInstruction(object)
> 
>       # fields .pc and .number
> 
> class FullRecordInstruction(RecordInstruction)
> 
> class BtraceInstruction(RecordInstruction)
> 
>       # field .error
> 
> Client python code can get a list of RecordInstruction to process, without
> being aware which method is used.
> 
> Use gdb.find_pc_line (pc) to get gdb.Symtab_and_line, so we don't need to
> have BtraceInstruction.sal.
> 
> BtraceInstruction .data, .decoded, .size, and .is_speculative, shouldn't be in
> BtraceInstruction.  They are about instruction.  We need to put them into a
> separate class, like gdb.Instruction, and add some function to get
> gdb.Instruction from pc.
> 
> I'll give comments to BtraceFunctionCall later.
> 
> --
> Yao (齐尧)
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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]