Re: GDB Console

[Dodji Seketeli <dodji seketeli org>]

Fabien Parent <parent f gmail com> a écrit:

> Hello,
>
> I'm working on the console for a while now, so I decided to send my
> current work on the mailing-list for comments on the general design.

Thank you for working on this, it's very appreciated.

I like the general orientation of the patch.  As usual, I have some
comments here and there on some specific organizational bits.  I didn't
go too much further because I understand that this is Work In Progress.

So here are my comments.

> +++ b/src/common/nmv-console.cc

[...]

> +struct Console::Stream::Priv {
> +    int fd;
> +
> +    Priv (int a_fd) :
> +        fd (a_fd)
> +    {
> +    }
> +
> +    void
> +    write (const std::string &a_msg) const
> +    {
> +        THROW_IF_FAIL (fd);

Maybe there should be a default constructor for this Priv type that
initializes fd to zero.  That way, if someone initializes Priv with
the wrong (default) constructor, this assert will be violated.
Otherwise, right now, if Priv is initialized with the default implicit
constructor (that is automatically generated by the compiler) fd is
going to be randomly initialized, and this assert might not catch
that.

[...]


> +struct Console::Priv {
> +    std::map<std::string, Console::Command&> commands;
> +    std::vector<Console::Command*> commands_vector;
> +
> +    int fd;
> +    struct readline_state console_state;
> +    struct readline_state saved_state;
> +
> +    Console::Stream stream;
> +    Glib::RefPtr<Glib::IOSource> io_source;
> +    sigc::connection cmd_execution_done_connection;
> +    sigc::connection cmd_execution_timeout_connection;

It seems to me this console type is too coarse grained.  I mixes the
concepts of a "terminal" and that of a "commands interpreter" in an
untangled manner.

It would be nice, for instance if the command line interpreter could
be a well defined type, that could be used without having anything to
do with readline, command line completion or terminal file
descriptors.  It would just execute a string of characters (which is
the serialized form of a command) and emit appropriate signals when
that execution is done.  I guess that command line interpreter would
be in src/dbgengine.

(DBG)Console would be a type that knows how to listen to user input
from the Vte terminal component, hooking the whole thing to the event
loop and providing command line completion goodies using readline.
Strings typed in that Console would be handed to the command line
interpreter that the Console would "own".

The separate, "atomic", command line interpreter type could also be
used to write fine grained regression tests where we are only
interested in command execution, not terminal handling plumbing:

    void on_debugger_stopped (IDebugger::StopReason reason,
			      bool has_frame,
			      const IDebugger::Frame& frame,
			      int thread_id,
			      int bp_num,
			      const UString &cookie);
    CmdInterpreter c;

    c.get_debugger_iface ()->stopped_signal ().connect (&on_debugger_stopped);
    c.execute ("load fooprog");
    c.execute ("break main");
    c.execute ("run");


[...]

> +++ b/src/common/nmv-console.h

[...]


> +class Console {
> +    struct Priv;
> +    SafePtr<Priv> m_priv;
> +

[...]

> +    class Command {
> +        sigc::signal<void> m_done_signal;
> +

[...]

> +        virtual const char** aliases () const

Why not use a reference to a vector of strings here, instead of a
char**?  I would think it's safer.

[...]


> +    typedef Command AsynchronousCommand;
> +    struct SynchronousCommand : public Command{
> +        virtual void operator() (const std::vector<std::string> &a_argv,
> +                         Stream &a_output)
> +        {
> +            execute (a_argv, a_output);
> +            done_signal ().emit ();

Hmmh.  Is there such a thing as a synchronous command in our setup?

Shouldn't the command interpreter instead listen to the
IDebugger::command_done_signal () and emit a "done" signal at that
time?  Which makes me think that having a done signal emitted by the
Command itself is probably not a as good as having the command
interpreter emit a "done" signal instead, passing the instance of
Command that was executed in argument.

[...]

> +++ b/src/dbgengine/nmv-dbg-console.cc

[...]

> +struct DBGConsole::Priv {
> +    DebuggingData data;
> +
> +    CommandContinue cmd_continue;
> +    CommandNext cmd_next;
> +    CommandStep cmd_step;
> +    CommandBreak cmd_break;
> +    CommandPrint cmd_print;
> +    CommandCall cmd_call;
> +    CommandFinish cmd_finish;
> +    CommandThread cmd_thread;
> +    CommandStop cmd_stop;
> +    CommandNexti cmd_nexti;
> +    CommandStepi cmd_stepi;
> +    CommandOpen cmd_open;


Why is it useful to have these members here?  I think the command
interpreter should have a vector of (smart) pointers to Command.  This
type would have the command line interpreter as member, so that ....

[...]

> +DBGConsole::DBGConsole (int a_fd, IDebugger &a_debugger) :
> +    Console (a_fd),
> +    m_priv (new Priv (a_debugger))
> +{

... here, we'd just instantiate each command and call the register
function of the command interpreter, passing it along the smart
pointer to the Command.

[...]


> +++ b/src/persp/dbgperspective/nmv-dbg-perspective.cc

[...]

>      Gtk::Box& get_terminal_box ();
> @@ -897,6 +904,9 @@ struct DBGPerspective::Priv {
>      Path2MonitorMap path_2_monitor_map;
>      SafePtr<LocalVarsInspector> variables_editor;
>      SafePtr<Gtk::ScrolledWindow> variables_editor_scrolled_win;
> +    SafePtr<Terminal> console_terminal;
> +    SafePtr<DBGConsole> dbg_console;

I think the terminal should be an implementation detail of the console
here.  So just having the DBGConsole at this point should be enough.

Thanks again for hacking on this.

-- 
		Dodji