Re: record-btrace

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

On Thu, 31 Jan 2013 16:38:43 +0100, Metzger, Markus T wrote:
> The problem I have with "target record" is that the sub-commands are added in
> add_target, one per added target. Would it be OK to export targetlist so I can add
> an alias for target "record-full"? Or is it OK to just drop target "record"?

OK, let's export targetlist, with some comment it should not be normally used.


> If we dropped target record, we would break backwards compatibility.

With eclipse-cdt-8.1.0-1.fc17.x86_64 I see it uses command "record" and not
"target record".


> There's a single test in gdb.mi that would need to be adjusted, but I would
> also expect that GUIs would be broken.

OK, both "record" and "target record" can be kept compatible, I do not mind,
you are right it may cause front ends compatibility issue.


> On a similar matter, I renamed the existing "set/show record" subcommands by
> prepending "full-", i.e. "insn-number-max" becomes "full-insn-number-max".

Could it be a prefix command? "set record full insn-number-max" etc.

To match "record full" (vs. "record btrace").


> This does not break any tests but would affect scripts and MI.

There should be aliases with deprecate_cmd during such changes, deprecate_cmd
also ensures it does not get <tab>-completed anymore.


> For record-btrace and record-full, to_record_list will be quite different. For all
> other targets, it will be NULL.
> 
> We could share to_disconnect, to_detach, to_kill, and to_mourn_inferior. They
> are just forwarding the request after unpushing the record target.
> 
> I made the "record stop" command generic. It searches for a record target
> beneath the current and unpushes the first it finds. I could do something
> similar for the above.

It looks OK to me, it would be better to see the code.


> There's a record_changed notifier that is called in to_open and in the stop
> command. Why is it not called in to_close instead of in the stop command?

to_close is a bit dangerous that the target is no longer on stack, as
documented in unpush_target.  So maybe record_changed could want to do
something with the record traget yet.

But with the currently only listener MI it should work even from to_close.


> As long as you know what you are doing, it's convenient to be able to read
> variables that you know have not changed. You need to be very careful, though,
> to consider also stack changes when you reverse-step into a different function.
> Things get worse if the compiler generates location lists.

As there should be the custom unwinder blocking unwind the current frame won't
be found there to access local variables, they should print an error.


> > > I can do the CLI warning as first step. Will this warning also be available
> > > via MI?
> > 
> > It will be printed as general GDB output record but at least Eclipse CDT users
> > won't notice it.  It gets displayed only after selecting Debug window -> "gdb"
> > and then it is in the displayed window "Console" as a "warning: ..." text.
> 
> We should maybe make this configurable so GUIs will not show wrong data.

I believe it is up to GUI to handle it somehow specially.  The GUI knows it is
currently operating in history, Eclipse already has special support for
"record".


Thanks,
Jan