tab completion for process signal

[MrHate]

Summary

Provide a list of Unix signals for the tap completion for command "process signal".

[teemperor]

I think that's a good start, but the usual way LLDB implements completions is that we go to the same backend that handles the actual values and retrieve the completions from there. The main reason is that this prevents duplicate code and that we provide completions that don't end up being valid commands. For example, you are missing SIGRTMIN+12 in your list but MIPS on Linux has this signal. So we won't get completions for that. But if you add SIGRTMIN+12 to your list, then on other systems we would offer it as completion even though it's not a valid signal there. We could of course implement all the checks for that in the HandleArgumentCompletion function but then we end up with a lot of duplicate and hard to maintain code.

So the better way to implement this is to look at the UnixSignals class and just list all the signal names in it. You can get the current instance of the UnixSignals via process->GetUnixSignals().

In general you can always look how DoExecute is implemented for a CommandObject and then mirror that logic but instead of doing the actual action just extract the completions. That works for like 99% of the completions in LLDB I would guess (even though some might require some refactoring to get it into a nice format).

[MrHate]

Thanks for pointing out my misunderstanding on the Unix signals, and fetching the list of valid signals from backend is always a better way indeed.
However, getting the current process from the current context m_exe_ctx which is got from m_interpreter will cause an issue that the completion won't be triggered before executing some process commands.
This is similar to frame variable cause these two commands both work on the base of the current context from m_interpreter.
In order to process completion successfully whenever we've got a valid process, I will update the current execution context both in m_interpreter and the current command object.
But what I am worried about is whether a new issue would be caused due to updating the current execution context.

[MrHate]

Update the current execution context, then get signals from the current process.

[MrHate]

removed unnecessary assignment in test

[labath]

In D75418#1902266, @MrHate wrote:

In order to process completion successfully whenever we've got a valid process, I will update the current execution context both in m_interpreter and the current command object.
But what I am worried about is whether a new issue would be caused due to updating the current execution context.

This worries me too. I don't really understand how this works, but it seems rather odd that this command should need to do this. There's plenty of other commands that operate on the current process and they don't call UpdateExecutionContext. Maybe @jingham knows what is going on here?

[jingham]

I think this is just a bug in argument completion.

The CommandInterpreter needs to sync up with the currently selected target/process/thread/frame before running commands.

It COULD do that by having every API that changes the selected T/P/T/F above notify the CommandInterpreter of the change. Then you would only need UpdateExecutionContext when you want to override the currently selected context. You do that, for instance, when you run stop hooks or breakpoint commands, which don't actually select the context they operate on.

But it doesn't do it that way. Instead, the CommandInterpreter is responsible for syncing its state before running commands. It calls UpdateExecutionContext on each command evaluation, either passing along an override context as above, or sending in a nullptr to tell it to sync with the currently selected state.

However, HandleCompletions doesn't do that, so it might not be operating on a consistent state. To be consistent, it really should do that as well.

This is actually a bug in the other completions that actually depend on a selected entity. For instance, run a program to a breakpoint and before doing anything else do: frame var l<TAB> and you won't get anything, but then do just frame var and then frame var l<TAB> and then you will get all the locals starting with l... You wouldn't generally notice this, any other command you issued after stopping would have sync'ed up the state. So it only happens when frame var l<TAB> is the FIRST thing you do after a stop.

Seems like the correct fix is to have HandleCompletions call UpdateExecutionContext, rather than having individual completions call it.

[MrHate]

Removed individual execution context updating.

[teemperor]

I would like to have a test for this without a running process but otherwise this LGTM. Thanks a lot!

lldb/test/API/functionalities/completion/TestCompletion.py

97

Could you make another test case that is trying tab completion without a running process? That should just provide no completions and not crash or something like that.

[MrHate]

Added a test case where test "process signal" without a running process.

[teemperor]

LGTM, thanks!

lldb/source/Commands/CommandObjectProcess.cpp

CommandObjectProcessSignal(CommandInterpreter &interpreter)
     arg.push_back(signal_arg);
 
     // Push the data for the first argument into the m_arguments vector.
     m_arguments.push_back(arg);
   }
 
   ~CommandObjectProcessSignal() override = default;
 
+  void
+  HandleArgumentCompletion(CompletionRequest &request,
+                           OptionElementVector &opt_element_vector) override {
+    if (!m_exe_ctx.HasProcessScope() || request.GetCursorIndex() != 0)
+      return;
+
+    UnixSignalsSP signals = m_exe_ctx.GetProcessPtr()->GetUnixSignals();
+    int signo = signals->GetFirstSignalNumber();
+    while (signo != LLDB_INVALID_SIGNAL_NUMBER) {
+      request.AddCompletion(signals->GetSignalAsCString(signo), "");
+      signo = signals->GetNextSignalNumber(signo);
+    }
+  }
+
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Process *process = m_exe_ctx.GetProcessPtr();
 
     if (command.GetArgumentCount() == 1) {
       int signo = LLDB_INVALID_SIGNAL_NUMBER;
 
       const char *signal_name = command.GetArgumentAtIndex(0);

lldb/test/API/functionalities/completion/TestCompletion.py

def test_process_attach_dash_dash_con(self):
             'process attach --con',
             'process attach --continue ')
 
     def test_process_launch_arch(self):
         self.complete_from_to('process launch --arch ',
                               ['mips',
                                'arm64'])
 
+    def test_process_signal(self):
+        # The tab completion for "process signal"  won't work without a running process.
+        self.complete_from_to('process signal ',
+                              'process signal ')
+
+        # Test with a running process.
+        self.build()
+        self.main_source = "main.cpp"
+        self.main_source_spec = lldb.SBFileSpec(self.main_source)
+        lldbutil.run_to_source_breakpoint(self, '// Break here', self.main_source_spec)
+
+        self.complete_from_to('process signal ',
+                              'process signal SIG')
+        self.complete_from_to('process signal SIGA',
+                              ['SIGABRT',
+                               'SIGALRM'])
+
     def test_ambiguous_long_opt(self):
         self.completions_match('breakpoint modify --th',
                                ['--thread-id',
                                 '--thread-index',
                                 '--thread-name'])
 
     def test_plugin_load(self):
         self.complete_from_to('plugin load ', [])