Re: record-btrace

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Markus,

thanks for a nice plan overview.


On Mon, 28 Jan 2013 17:13:59 +0100, Metzger, Markus T wrote:
> 1. add a new command "record-btrace" that pushes a new

"record btrace", there are already subcommands of "record".

It would be unconvenient to have some subcommands as "record-ABC" and some as
"record DEF" wrt tab-completion.

On Thu, 20 Dec 2012 08:17:26 +0100, Jan Kratochvil wrote:
# Currently one uses just "record" to start the full-recording.  I was expecting
# it could print an error and one would use in newer versions of GDB
# "record btrace" vs. "record full" or something like that.

Expecting in this mail the current record backend would be called "full",
I do not have a better name now.  "slow" does not seem to great. :-)


> record_btrace_target_ops onto the target stack. By default, this will enable
> branch tracing for all existing and all new threads. Arguments may later be
> added to only enable branch tracing selectively for some threads.

OK.


> The existing brace target-ops and also the btrace RSP packets will need to
> remain since I still need to configure branch tracing on the target. Without
> them (i.e. the functionality hard-coded in record_btrace_target_ops),
> I don't see how I could handle the remote case. Record_btrace_open will call
> target_btrace_enable to do the actual work. It will fail if one of the
> btrace enable calls fails.

OK; therefore two btrace struct target_ops for linux-nat.c vs. remote.c and one
btrace struct target_ops for record btrace (vs. full).


> The target is global. The data (i.e. configuration, history), however, will
> still be per-thread. I don't know how target record handles this.

record.c has global variables which is sure wrong.


> I was thinking of keeping the btrace internal structure.

Yes; FYI struct thread_info could have REGISTRY_FIELDS for dynamic data with
functions like register_thread_info_data_with_cleanup.  But currently it does
not have REGISTRY_FIELDS so the current struct thread_info->btrace is OK.


> If we're going to extend this in the future, say, by adding an LBR-based
> branch trace, this would be another record_btrace_lbr_target_ops that may
> share some of the functions with the BTS-based record-btrace target.

Probably, unaware now how much the LBR backend will differ.


> You mentioned that we should not pop that target when the process terminates
> to implement auto enabling for new processes. I'm not quite clear how that
> will work. I always thought the target stack is newly setup for each
> process. I wonder how we can leave the topmost entry always on?

It is rather the opposite.  Targets hook on to_mourn_inferior and call
unpush_target on themselves.


> 2. implement the standard "record" commands for this new record-btrace
> target, i.e. "info record", "set/show record", "record goto", "record save",
> "record restore", "record stop", "record delete".
> 
> The "set/show record" sub-commands won't make much sense for record-btrace.
> On the other hand, we would need some other sub-commands to configure branch
> tracing. Would I dynamically add and remove those (sub-)commands depending
> on which record target is currently active? I have not seen functions to
> remove commands. Or would I need to put sub-command handling into a single
> "set record" and "show record" command (i.e. do not use sub-commands)?

For example current "set record insn-number-max" should be moved to
"set record full insn-number-max" and current "set record insn-number-max" made
an alias to it with deprecate_cmd.

You are right dynamically adding/removing commands is not used in GDB.  They
are added as long as the module is compiled in during GDB configure and
commands are never removed.

It would be good to protect these commands
like "set record full insn-number-max" and the btrace ones so that if other
record target is currently pushed on stack they print a warning that the new
value is ignored with the currently active record backend.


> The "info record" command makes sense for all record variants, but it will
> show different information for each record target. Would I add a new
> target-ops function for it? Or would I add a new struct record_ops to
> collect those functions?

As there will be already pushed/unpushed target_ops for the backend I find
easier to make new target_ops methods for such dispatching.

This abstract part of record.c should be moved to a separate file.
Or rather the "full" backend to be moved from record.c to record-full.c and
record.c to remain only a thin wrapper for some of these record parts common
across the btrace+full record backends.

"target record" could be also renamed to "target record-full" (and "target
record" kept as deprecated_cmd alias).


> The "record save/restore" commands won't be available. Needs more target-ops
> or record-ops functions.

Yes, so they should also use targets_ops method and the btrace backend will
print an error for them.


> The "record goto" and "record delete" commands will need to work on btrace
> data structures, instead. Needs more target-ops or record-ops functions.

Yes, new target_ops methods seem OK to me.


> I would essentially move those commands into a new file and implement some
> record abstraction for them to work on.

I see you probably describe what I have described above.


> 3. add a new "record disas"

"record disassemble" (the shortening is done automatically by GDB commands
parser).


> sub-command to print the disassembly of the last
> instructions and iterate through the instruction history similar to the list
> command. This will be disabled for target record and implemented for target
> record-btrace.

"disabled" = printing an error.


> I would add another target_ops or record_ops function to do the actual work
> that is called from the "record disas" command.
> 
> In a later step, this could be implemented for target record, as well.

Yes.


> 4. add a new "record list" sub-command similar to "record disas" but listing
> the source lines that have been executed.

Extending the functionality also for the record-full target can be thought
afterwards.


> 5. extend "record stop" to take parameters that allow selective disabling of
> branch tracing. Not sure if and how this will work with the existing target
> record.

Existing record-full does not have any selection so it would print an error on
such attempt.


> 6. Implement reverse execution for record_btrace_target_ops:
> 
> To_resume/to_wait: iterate through instruction history
> To_insert_breakpoint: maintain breakpoint list - don't forward to target
> To_fetch_registers: only support the instruction pointer register
> To_store_registers: not allowed
> To_xfer_partial: not allowed
> To_~_watchpoint: not allowed
> 
> When I don't allow reading memory in target record, nothing works, anymore,
> i.e. almost all gdb commands fail.
> When I don't allow fetching registers in target record, there seems to be no
> impact - I guess the regcache is up-to-date so we don't really need to ask
> the target.
> When I do a single reverse stepping command, however, it fails and gdb
> believes that the target is executing - nothing works, anymore.
> 
> The first experiment is not very encouraging. Do you think this can be done,
> at all?
> 
> I may need to allow reading memory for read-only dynamic sections so that at
> least "disas" works. Does gdb maintain a dynamic section map? Could gdb be
> taught to read this memory from a file, instead?

I would not be too strict with accessing the inferior memory.  I understand
that the memory content may be different when GDB is in the btrace history but
most of the memory including read/write variables a user may want to read will
be the same.

It could rather just print a CLI (the default command-line interface) warning
if one accesses a read/write memory during command execution.

Forbidding any memory access may be correct but it may be pain for the users.


> I may also need to replace the unwinder even though I'm not adding unwind
> support, yet - just to prevent it from failing.

I agree, that would be too confusing.


> Are unwinders global or can
> a target add/remove unwinders? If unwinders can't be added/removed
> dynamically, I would add an unwinder to target_ops that will, if present,
> precede all other unwinders. Would that be OK?

You can use frame_unwind_prepend_unwinder to place it as the first unwinder.

You do not need to remove it, struct frame_unwind->frame_sniffer_ftype sniffer
can just always return 0 when the record-btrace backend is currently not
pushed on the target stack.  Therefore next unwinder will be queried in such
case.


> I would like to simulate the above two (i.e. allow access to read-only
> memory and add a do-nothing unwinder) to see what issues we might be running
> into with a target record-btrace. Would you help me with that?

Yes, still I hope it is more clear now.


> Reverse- and subsequent forward- stepping command should target the current
> thread only as long as the thread is inside the recorded execution history.
> I hope that's what I get when I overwrite to_resume and to_wait and only
> consider the current thread.

BTW this behavior normally depends on "set scheduler-locking off/step/on".

to_resume's 3rd parameter STEP will be 0 if one does "continue".

<offtopic a bit, just for illustration>
FSF GDB has default "set scheduler-locking off" which I (and IIRC users) find
confusing so for example Fedora GDB has default "set scheduler-locking step".
I believe FSF GDB should also default to "set scheduler-locking step" but it
causes some other issues which is why it is not so.
</offtopic a bit>

I do not see how to implement scheduler-locking-aware implementation of the
btrace backend.

So I agree it is OK it would behave as if "set scheduler-locking on" was set.


> When they reach the end of the trace, they turn into normal stepping
> commands, i.e. I will forward to_resume and to_wait to the target beneath.
> This should implicitly continue other threads depending on the execution
> mode. When the target stops next time, the tracing positions for all stopped
> threads will be reset - I won't be able to find the old position in the new
> version of the trace. This behavior might be somewhat surprising if you
> switched threads while traversing the history.

I would rather print an error in such case - that one should switch to the
thread with position in history and roll it back to the current position
before the whole process can be resumed by the target beneath.

But I really do not mind how this special case is handled, IMO it won't happen
in any common case.


Thanks,
Jan