jimingham 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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
