[lldb/Plugins] Add lldb.ScriptedProcess.get_most_relevant_thread_index interpreter method

[mib]

Summary

This patch adds a new interpreter scripted process method to return the
most relevant thread index, which can be chosen aritrarily by the user.

This can be very helpful to ensure lldb will stop on the thread the
user judged the most relevant, in his script.

Signed-off-by: Med Ismail Bennani <[e-mail]>

[jingham]

This seems wrong to me. Normal Processes don't get to choose which thread the generic "which stop event is most important" logic selects, they just report the stop events and then the higher level logic decides which one to select. It seems more symmetric to have the ScriptedThreads work the same way - after all they are free to assign whatever StopInfo they want to each stop. Why do you need to force the result in the ScriptedProcess case?

[mib]

In D118482#3279843, @jingham wrote:

This seems wrong to me. Normal Processes don't get to choose which thread the generic "which stop event is most important" logic selects, they just report the stop events and then the higher level logic decides which one to select. It seems more symmetric to have the ScriptedThreads work the same way - after all they are free to assign whatever StopInfo they want to each stop. Why do you need to force the result in the ScriptedProcess case?

This is completely optional. The user doesn't really have to override the base method, in which case lldb won't force the ScriptedProcess to select a specific thread. It will use its current stopping logic to look at the ScriptedThreads stop reasons and determine which one it should select.

However, in some cases (i.e. crashlogs), we only have very sparse metadata to construct the ScriptedThreads, so having the ability to select the most relevant thread to the user can be quite helpful.

Also, I'm not sure a Scripted Process should share the exact same behavior as a Normal Process as they will eventually be able to report fully synthetic threads / frames, but I might be wrong on that.

[jingham]

My preference for things like crash reports would be that you make up a stop reason that's significant for the thread(s) you think are interesting and then let lldb figure out how to display them. After all, if you set a selected thread w/o having a StopInfo that would warrant selecting that thread, it's going to look weird. We selected thread 100, but it hadn't stopped for any particular reason is an odd experience.

And I don't think the default behavior should be to select thread 1, as that forces the upper layers hand.

[mib]

Address @jingham concerns by making this fully optional:

• Change python default value to 0. Thread indices in lldb are 1-based, so 0 sounds like a good error_value.
• Check for python returned integer and replace it by LLDB_INVALID_INDEX32 to prevent ScriptedProcess::RefreshStateAfterStop to select a thread with it.

[shafik]

Just nitpicks

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

362 ↗

(On Diff #404068)

const

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

123 ↗

(On Diff #404068)

const

[mib]

Address @shafik feedback

[mib]

Following an offline discussion with @jingham, we agreed that we should leave the internal lldb machinery deal with selecting the right thread to be stopped at.

However, the interpreter ScriptedProcess method to let the user provide the most relevant thread index can still be very useful, for instance when creating the appropriate stop reason from the ScriptedThread instances to fetch the most_relevant_thread_index from ScriptedProcess.

This is why this update removes all the thread selecting logic from the ScriptedProcess plugin and makes it so get_most_relevant_thread_index cannot be accessed from the debugger.

[jingham]

As an implementation detail for stack_core_scripted_process this seems fine. I'm not sure whether it belongs in the generic scripted_process.py example however. If you do have reasons for keeping it there as well, I think you need to say what it means for a thread to be the "most relevant" from a scripted process's point of view.

[mib]

@jingham I just got rid of all of this, and added a is_stopped variable to the stack_core_scripted_process.StackCoreScriptedThread class in D118484.

lldb/examples/python/scripted_process/scripted_process.py

def get_thread_with_id(self, tid):
 
         Returns:
             Dict: The thread represented as a dictionary, with the
                 tid thread ID. None if tid doesn't match any of the scripted
                 process threads.
         """
         pass
 
+    def get_most_relevant_thread_index(self):
+        """ Get the index of the most releveant scripted process thread.
+
+        Returns:
+            Int: The most relevant thread index according to the user.
+                 (default: 0)
+        """
+        return 0
+
     @abstractmethod
     def get_registers_for_thread(self, tid):
         """ Get the register context dictionary for a certain thread of
             the scripted process.
 
         Args:
             tid (int): Thread ID for the thread's register context.
 

lldb/test/API/functionalities/scripted_process/stack_core_scripted_process.py

def read_memory_at_address(self, addr: int, size: int) -> lldb.SBData:
             return data
 
         data.SetDataWithOwnership(error, bytes_read,
                                   self.corefile_target.GetByteOrder(),
                                   self.corefile_target.GetAddressByteSize())
 
         return data
 
+    def get_most_relevant_thread_index(self):
+        return 3
+
     def get_loaded_images(self):
         # TODO: Iterate over corefile_target modules and build a data structure
         # from it.
         return self.loaded_images
 
     def get_process_id(self) -> int:
         return 42