RE: GDB 8.0 release/branching 2017-03-20 update

["Wiederhake, Tim" <tim dot wiederhake at intel dot com>]

Hi Yao,

> -----Original Message-----
> From: Yao Qi [mailto:qiyaoltc@gmail.com]
> Sent: Friday, April 7, 2017 10:11 AM
> To: Wiederhake, Tim <tim.wiederhake@intel.com>
> Cc: Joel Brobecker <brobecker@adacore.com>; Metzger, Markus T
> <markus.t.metzger@intel.com>; gdb-patches@sourceware.org; xdje42@gmail.com
> Subject: Re: GDB 8.0 release/branching 2017-03-20 update
> 
> "Wiederhake, Tim" <tim.wiederhake@intel.com> writes:
> 
> Hi Tim,
> I was off yesterday.  Thanks for the summary.
> 
> > The Python interface shall look like this (from Python's perspective):
> >
> > gdb:
> > 	Record		start_recording([str method], [str format])
> > 	Record		current_recording()
> > 	None		stop_recording()
> >
> > 	Unchanged.
> >
> > gdb.Record:
> > 	str		method
> > 	str		format
> > 	RecordInstruction	begin
> > 	RecordInstruction	end
> > 	RecordInstruction	replay_position
> > 	RecordInstruction[]	instruction_history
> > 	RecordFunctionSegment[]	function_call_history
> > 	None		goto(RecordInstruction insn)
> >
> > 	gdb.Record loses the "ptid" attribute.  "instruction_history" and
> > 	"function_call_history" actually returns a "list" or "list-like
> > 	object"
> > 	as there is no way to enforce the type of elements in this list on a
> > 	language level in Python.  Practically, these list will always
> > 	contain
> > 	"RecordInstruction" / "RecordFunctionSegment" objects since we
> > 	control
> > 	their creation.  "*_history" may return "None" if the current
> > 	recording
> > 	method cannot provide such a list.
> >
> > gdb.Instruction:
> > 	int		pc
> > 	buffer		data
> > 	str		decode
> > 	int		size
> >
> > 	gdb.Instruction is meant to be an abstract base class.  The user
> > 	will
> > 	never receive a raw gdb.Instruction object and accessing any
> > 	attribute
> > 	in this class will throw a "NotImplementedError" (from what I
> > 	understand, that's the preferred way to handle this kind of
> > 	situation
> > 	in Python).
> From the implementation point of view, it can be an abstract base class,
> but we don't have to mention it in Python/Document.  Likewise, we don't
> apply the restriction that "user will never receive a raw
> gdb.Instruction object" on the Python interface.

You are correct, this is not meant to end up in the documentation.  Instead,
this is just the explanation to see if I understood the purpose of
gdb.Instruction correctly.

> >
> > gdb.RecordInstruction (gdb.Instruction):
> > 	int		pc	<inherited from gdb.Instruction>
> > 	buffer		data	<inherited from gdb.Instruction>
> > 	str		decode	<inherited from gdb.Instruction>
> > 	int		size	<inherited from gdb.Instruction>
> > 	int		error
> > 	gdb.Symtab_and_line	sal
> > 	bool		is_speculative
> >
> > 	gdb.RecordInstruction is a sub class of gdb.Instruction. It loses
> > 	the "number" attribute.  "error" will be "None" if there is no
> > 	error.
> >
> > gdb.<Whatever>Instruction (gdb.Instruction):
> > 	int		pc	<inherited from gdb.Instruction>
> > 	buffer		data	<inherited from gdb.Instruction>
> > 	str		decode	<inherited from gdb.Instruction>
> > 	int		size	<inherited from gdb.Instruction>
> > 	...
> >
> > 	Created by other Python interfaces, e.g. a function that
> > 	dissasembles
> > 	target memory.
> Right, to be clear, we focus on record/btrace instruction.

Correct.  I mentioned this only to showcase how gdb.Instruction would be used by
other Python interfaces.

> > gdb.RecordFunctionSegment:
> > 	gdb.Symbol	symbol
> > 	int		level
> > 	gdb.RecordInstruction[]	instructions
> > 	gdb.RecordFunctionSegment	up
> > 	gdb.RecordFunctionSegment	prev
> > 	gdb.RecordFunctionSegment	next
> >
> > 	Renamed from "gdb.BtraceFunctionCall" to better fit the general
> scheme.
> 
> "Segment" is less clear than "Call".  Does this object represent
> something other than a function call?

Imagine you have some code like this:

 void
 a ()
 {
   /* some code */
 }
 
 void
 b ()
 {
   /* some code */
   a ();
   /* some code */
 }

Then the trace for a call to function "b" would generate three function call
segments: One for the code in "b" before the call to "a", one for the code in
"a" and one for the code in "b" after the call to "a" returned.

The "next" attribute in the first function segment points to the third function
segment and likewise the "prev" attribute in the third function segment points
to the first function segment.  For the second function segment the "up"
attribute points to the first function segment.

"Segment" is used instead of "Call" throughout the btrace code to avoid the
impression that one function segment contains the whole recorded data for a
function call whereas it only contains a part of it.  I believe we should
continue this terminology in Python.
 
> > 	Loses the "number" attribute.  As above, "instructions" actually
> returns
> > 	a list / list-like object.  "prev_sibling" and "next_sibling" are
> > 	renamed to "prev" and "next" for simplicity (discussed with Markus
> > 	off-list).
> 
> It is a good renaming to me.
> 
> >
> > Correct so far?
> >
> 
> Yes, I think so :)

Glad to hear :)

> > Initially I supported the idea of losing the "number" attributes in
> > "gdb.RecordInstruction" and "gdb.RecordFunctionSegment", but I see a
> > problem with that now:  If an instruction is executed multiple times
> > (e.g. in a
> > loop), all user visible attributes for these gdb.RecordInstruction
> > objects are
> > the same, nevertheless a comparison with "==" does not yield "True"
> > because they
> > represent, well, two different instances of execution of that
> > instruction.
> > Keeping the "number" attribute in would show the user, why those objects
> > are not
> > equal.  Therefore I propose to retain the "number" attribute in
> > "gdb.RecordInstruction" and for symmetry in "gdb.RecordFunctionSegment"
> > as well.
> 
> As far as I can see, it is not a problem to me.  The multiple instances
> of the same instruction are different, because the same instruction
> executed multiple times.  IOW, gdb.RecordInstruction from different
> slots of .instruction_history are different.

Then it's decided, and the "number" attribute may stay.

> > Markus and I further discussed how we handle gaps or other errors in the
> > trace
> > from the Python point of view.  We came to the conclusion that it would
> > be
> > beneficial for the user if we replaced the definition of
> > "gdb.RecordInstruction"
> > above with the following two:
> >
> > gdb.RecordInstruction (gdb.Instruction):
> > 	int		pc	<inherited from gdb.Instruction>
> > 	buffer		data	<inherited from gdb.Instruction>
> > 	str		decode	<inherited from gdb.Instruction>
> > 	int		size	<inherited from gdb.Instruction>
> > 	int		number
> > 	gdb.Symtab_and_line	sal
> > 	bool		is_speculative
> >
> > gdb.RecordGap:
> > 	int		number
> > 	str		error_message
> > 	int		error_code
> >
> > 	Does NOT inherit from gdb.Instruction.
> >
> > gdb.Record.instruction_history would then not "return a list of
> > RecordInstructions" but instead "return a list of RecordInstructions and
> > (potentially) RecordGaps".
> 
> Yeah, I can see the motivation of this change, because it is strange to
> have a field "error" in gdb.RecordInstruction to indicate this.

Exactly.  And it goes even further:  We don't have a "pc" or "size" for a gap,
so what do we return in this case?  That's why the gdb.RecordGap type does not
contain these fields that don't make sense in that case.  A gap is not an
instruction and we believe it makes things clearer if "gap" therefore does not
pretend to be an instruction.

> > The user needs to distinguish between instructions and gaps somehow
> > anyway, and
> > this solution would let them do so quite nicely.  Example code:
> >
> >  r = gdb.current_recording()
> >  for insn in r.instruction_history:
> > 	try:
> > 		print insn.pc, insn.sal
> > 	except:
> > 		# It's a gap!
> > 		print insn.error_message
> >
> I don't like using exception for control flow.

The above code with the "error" attribute still in place would look like this:

 r = gdb.current_recording()
 for insn in r.instruction_history:
   if not insn.error:
     print insn.pc, insn.sal
   else:
     print insn.error_message

The user has to take care about the rare possibility of a gap occurring in the
trace.  In the latter case, they have to explicitly check that the instruction
they look at is not a gap, which is easy to forget and will lead to strange
results as the "pc", "sal", "data", "size" and so on attributes of a gap do not
contain meaningful data.

In the former example, the exception is raised by accessing "insn.pc", an
attribute that does not exist for a gap.  I expect the usual code for the
"except" path to just contain a "pass" or "continue" statement.  I just printed
the error_message in the example to showcase how this situation would be handled
if a user desired so.

If you wanted to write this example without handling exceptions, you could do
so as well:

 r = gdb.current_recording()
 for insn in r.instruction_history:
	print getattr(insn, "pc", "has no address"), getattr(insn, "sal", "has no sal")

And even if the user simply wrote:

 r = gdb.current_recording()
 for insn in r.instruction_history:
	print insn.pc, insn.sal

there are two cases: The trace contains no gaps (common), or the trace contains
gaps (uncommon).  If the trace did not contain any gaps, the code works as
expected.  If the trace contained gaps, Python will raise an exception pointing
in the right direction of the problem.

If instead we continued to offer the "error" attribute and the user forgot to
check, the code would silently fail and produce wrong results.  I believe the
option where we fail loud and clear is favorable over the silently wrong result.

> If I understand "gap"
> correctly, it is caused something interrupt the tracing.

There might be other causes as well, such as overflows, decoding errors, etc.
Again, ideally the trace contains no gaps at all, but rarely it may contain one
or more gaps interleaved with the regular instructions.

>  I'd like to
> change the interface like this,
> 
> gdb.InstructionHistory
>   a list of gdb.RecordInstruction
>   gdb.RecordGap (or gdb.RecordStopReason)
> 
> It saves a list of instructions and why the record is stopped or
> interrupted.  It can be different reasons, like hardware limitation,
> or user preference (user only wants to record/trace instructions
> executed in current function foo, so any function calls in foo will
> cause a gap in the history.  In this way, gdb.RecordGap don't have to be
> an error).
> 
> gdb.Record.instruction_history returns a list of gdb.InstructionHistory.

Just to make sure: You propose that gdb.Record.instruction_history returns a
list of partial lists, and each partial list contains the reason why that
particular partial list was interrupted / stopped, correct?

Generating this list of lists would be quite expensive as we have to go over the
whole list of instructions (that at least for btrace internally /is/ a list of
instructions interleaved with gap objects), count the gaps and save their
position to know the length of this list of lists and the start and end of the
individual sub lists.  With the explanation above how to do this without having
to care about exceptions, would you agree on keeping the list one-dimensional,
with instructions and rarely gaps interleaved as-is?

Regards,
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