Re: [RFA 7/8] Add truncate_repeat_arguments function

[Pedro Alves <palves at redhat dot com>]

On 10/18/2017 04:47 AM, Tom Tromey wrote:
>>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes:
> 
> Yao> This global variable worries me a little bit.  Is it possible that
> Yao> different part of GDB request to execute the repeatable commands?  For
> Yao> example, CLI and MI issues command, or different UIs issue command.
> 
> It's hard to reason about, I agree.
> 
> I think commands can either be run via "cmd_func", or by calling the
> command function directly.
> 
> Direct calls can be directly audited -- and none of the callers of the
> new set_repeat_arguments function are called this way.
> 
> There are also not many calls to cmd_func; and at least when I look at
> these (I encourage you to check my work), I think only execute_command
> can possibly call a command that calls set_repeat_arguments.
> 
> Since execute_command uses a scoped_restore to save and restore this
> global, I think this must be ok.
> 
> It does seem a bit fragile.  For example, I'd recommend not adding new
> callers of set_repeat_arguments.  I'm open to suggestions for another
> way to approach this.

I think an unanswered question is whether the cases of two different
UIs or interpreters running a command that calls set_repeat_arguments
are handled correctly (for some definition of correctly).

Two UIs means doing something like:
  gdb -ex "new-ui console /dev/pts/56"
or:
  gdb -ex "new-ui mi /dev/pts/56"

and then typing "list foo" in main UI and then just "<ret>"
in the extra UI.

while two interpreters means something like:

  $ gdb -i=mi
  (gdb)
  -interpreter-exec console "CLI COMMAND"
  -equivalent-mi-command

I'm not sure there's any MI command that calls into
the commands that set repeat arguments.

For for different UIs: currently, while each UI has its own line buffer,
saved_command_line is still global.  I.e., the variable that holds the
command line that is passed to the command functions.  That means that if you
type a command in one UI, and then type "<ret>" in a second UI, the
second UI repeats the command typed in the first UI.  I don't recall a
good reason for that, other than "we can improve things incrementally on
top of the initial support for multiple UIs" and "one console and one
MI is the main use case".

I think this patch doesn't affect things here.  Even if we make
saved_command_line per-UI, it seems like repeat_arguments can still be
a global, since it's only used within the scope of a single execute_command
call.   It could be made per-UI too, though it wouldn't make much of a
difference, I think.

To expand a bit: It's entirely possible that execute_command ends up doing
another execute_command inside.  For example when running a GDB script.
And it's plausible that the nested execute_command runs in a different UI from
the one that started the script.  E.g., if a breakpoint is hit, and that
breakpoint has commands -- currently all run control events are always run with
the main UI as current UI, while the script that started the target running
could have been invoked from a different UI.   Actually, I noticed that
breakpoint commands aren't (they're run in inferior_event_handler, while
it's fetch_inferior_event that sets the current UI to the main UI right
at the top), which seems like a bug to me.  But in any case,
breakpoint commands run with from_tty == 0, so shouldn't be affected.

So I think the patch is OK.

BTW, while writing the above, I spent a bit playing with actually 
making saved_command_line be per-UI, and then also the
"list", "x", and "show command" global state.  Playing with 

  gdb -ex "new-ui console TTY" --args ./gdb

it seems to work nicely and intuitively.  See attached
(untested) patch.  I've pushed it to the 
users/palves/per_ui_repeat branch on sourceware too.
(And what I was trying to determine: ) it doesn't look
like your patch would affect doing this.

Thanks,
Pedro Alves

Attachment: per-ui-info
Description: Text document