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