[Lldb-commits] [lldb] [lldb] Implement basic support for reverse-continue (PR #99736)

Tue Jul 23 10:16:22 PDT 2024

jimingham wrote:

More generally, I think it will be more natural if reverse and forward continuations to be as much as possible "the exact same execution control machinery with a direction" not separate "forward" and "reverse" facilities.  So I'd rather we not start off separating them artificially.

Jim

> On Jul 23, 2024, at 10:02 AM, Jim Ingham ***@***.***> wrote:
> 
>> 
>> 
>> 
>>> On Jul 23, 2024, at 9:47 AM, Greg Clayton ***@***.***> wrote:
>>> 
>>> 
>>> @clayborg requested changes on this pull request.
>>> 
>>> In lldb/include/lldb/API/SBProcess.h <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688374284>:
>>> 
>>> > -  lldb::SBError Continue();
>>> +  lldb::SBError Continue(RunDirection direction = RunDirection::eRunForward);
>>> Our public API has rules:
>>> 
>>> can't change any existing API call, you can add an overload, but you can't change any public APIs that are already there. Anything in the lldb namespace can't be changed.
>>> no virtual functions
>>> can't change any ivars or the size of the object
>>> That being said, I would rather have a:
>>> 
>>> lldb::SBError ReverseContinue();
>>> call than have everyone using the Continue API to say wether they want to go forward or in reverse.
>>> 
>>> In lldb/include/lldb/Target/Process.h <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688376419>:
>>> 
>>> > -  Status Resume();
>>> +  Status Resume(lldb::RunDirection direction = lldb::eRunForward);
>>> internal APIs, anything not in the lldb namespace can be changed. So this is ok. Though I personally would like to see a:
>>> 
>>> Status ReverseResume();
>>> I am open to feedback from other here as my mind can easily be changed.
>>> 
>> You just have to omit the default argument, and leave the 0 argument form in place.  That will have the effect you want, all previous code will use the forward continue, and new code that wants to be explicit can pass the argument.
>> 
>> I asked for this change, because it seems like where you are going to use it you will have some variable telling yourself which direction you are going, and so if there's an argument, you will find yourself writing:
>> 
>> my_process.Continue(the_direction_parameter_i_already_had)
>> 
>> as opposed to
>> 
>> if the_direction_parameter_i_already_had == lldb::eRunForward:
>>     my_process.Continue()
>> else:
>>     my_process.ReverseContinue()
>> 
>> The former seemed much nicer to me.
>> 
>> 
>>> 
>>> In lldb/include/lldb/Target/Process.h <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688378725>:
>>> 
>>> > @@ -3139,6 +3146,7 @@ void PruneThreadPlans();
>>>                             // m_currently_handling_do_on_removals are true,
>>>                             // Resume will only request a resume, using this
>>>                             // flag to check.
>>> +  lldb::RunDirection m_last_run_direction;
>>> Feel free to initialize this here so we don't have to change the constructor:
>>> 
>>> lldb::RunDirection m_last_run_direction = lldb::eRunForward;
>>> In lldb/include/lldb/lldb-enumerations.h <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688381600>:
>>> 
>>> > +/// Execution directions
>>> +enum RunDirection { eRunForward, eRunReverse };
>>> +
>>> If we don't add an overload to continue by adding SBProcess::ReverseContinue() then this should be kept internal and not in this header file. If you add a new SBProcess::Continue(lldb::RunDirection) overload then this is needed. I would prefer a dedicated SBProcess::ReverseContinue() function.
>>> 
>> 
>> Can you explain why?  It seems like that usage is going to be more verbose to no purpose, as I showed above.
>> 
>>> 
>>> In lldb/include/lldb/Target/Process.h <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688386332>:
>>> 
>>> > @@ -1129,10 +1129,15 @@ class Process : public std::enable_shared_from_this<Process>,
>>>    /// \see Thread:Resume()
>>>    /// \see Thread:Step()
>>>    /// \see Thread:Suspend()
>>> -  virtual Status DoResume() {
>>> +  virtual Status DoResume(lldb::RunDirection direction) {
>>> I would rather have a new DoResumeReverse() instead of changing this virtual API. Many modified files in this patch are a result of the process plugins having to add support for not supporting reverse resumes.
>>> 
>> I also asked for this change in the review.  Ever other API that does resume takes this parameter, its odd and asymmetric not to do so.  
>> 
>>> 
>>> In lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688387135>:
>>> 
>>> > @@ -90,7 +90,7 @@ class ProcessKDP : public lldb_private::Process {
>>>    // Process Control
>>>    lldb_private::Status WillResume() override;
>>>  
>>> -  lldb_private::Status DoResume() override;
>>> +  lldb_private::Status DoResume(lldb::RunDirection direction) override;
>>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>> 
>>> In lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688387212>:
>>> 
>>> > @@ -203,11 +203,17 @@ ProcessWindows::DoAttachToProcessWithID(lldb::pid_t pid,
>>>    return error;
>>>  }
>>>  
>>> -Status ProcessWindows::DoResume() {
>>> +Status ProcessWindows::DoResume(RunDirection direction) {
>>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>> 
>>> In lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688387312>:
>>> 
>>> > @@ -402,9 +402,16 @@ lldb_private::DynamicLoader *ProcessKDP::GetDynamicLoader() {
>>>  
>>>  Status ProcessKDP::WillResume() { return Status(); }
>>>  
>>> -Status ProcessKDP::DoResume() {
>>> +Status ProcessKDP::DoResume(RunDirection direction) {
>>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>> 
>>> In lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688387557>:
>>> 
>>> >    Log *log = GetLog(WindowsLog::Process);
>>>    llvm::sys::ScopedLock lock(m_mutex);
>>>    Status error;
>>>  
>>> +  if (direction == RunDirection::eRunReverse) {
>>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>> 
>>> In lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688387654>:
>>> 
>>> > @@ -52,7 +52,7 @@ class ProcessWindows : public Process, public ProcessDebugger {
>>>    Status DoAttachToProcessWithID(
>>>        lldb::pid_t pid,
>>>        const lldb_private::ProcessAttachInfo &attach_info) override;
>>> -  Status DoResume() override;
>>> +  Status DoResume(lldb::RunDirection direction) override;
>>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>> 
>>> In lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688388615>:
>>> 
>>> > @@ -111,7 +111,7 @@ class ProcessGDBRemote : public Process,
>>>    // Process Control
>>>    Status WillResume() override;
>>>  
>>> -  Status DoResume() override;
>>> +  Status DoResume(lldb::RunDirection direction) override;
>>> We should use DoResumeReverse() in Process.h and this change will add a new interface definition.
>>> 
>>> In lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688388968>:
>>> 
>>> > @@ -182,10 +182,17 @@ void ScriptedProcess::DidResume() {
>>>    m_pid = GetInterface().GetProcessID();
>>>  }
>>>  
>>> -Status ScriptedProcess::DoResume() {
>>> +Status ScriptedProcess::DoResume(RunDirection direction) {
>>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>> 
>>> In lldb/source/Plugins/Process/scripted/ScriptedProcess.h <https://github.com/llvm/llvm-project/pull/99736#discussion_r1688389053>:
>>> 
>>> > @@ -52,7 +52,7 @@ class ScriptedProcess : public Process {
>>>  
>>>    void DidResume() override;
>>>  
>>> -  Status DoResume() override;
>>> +  Status DoResume(lldb::RunDirection direction) override;
>>> We should use DoResumeReverse() in Process.h and this change won't need to happen.
>>> 
>>> —
>>> Reply to this email directly, view it on GitHub <https://github.com/llvm/llvm-project/pull/99736#pullrequestreview-2194496473>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW6GKSTRDQ44D76RPE3ZN2CKDAVCNFSM6AAAAABLFTBQAKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJUGQ4TMNBXGM>.
>>> You are receiving this because you are on a team that was mentioned.
>>> 
>> 
>> 

https://github.com/llvm/llvm-project/pull/99736