Skip to content

[lldb] Fix error that lead Windows to think it could reverse execute #137351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 25, 2025

Conversation

DavidSpickett
Copy link
Collaborator

The new test added in #132783 was failing on Windows because it created a new error to say it did not support the feature, but then returned the existing, default constructed error. Which was a success value.

This also changes the GDBRemote error message to the same phrasing used in all the other places so we don't have to special case any platform.

The new test added in llvm#132783
was failing on Windows because it created a new error to say it did
not support the feature, but then returned the existing, default
constructed error. Which was a success value.

This also changes the GDBRemote error message to the same phrasing used
in all the other places so we don't have to special case any platform.
@DavidSpickett DavidSpickett requested review from labath and removed request for JDevlieghere April 25, 2025 15:50
@llvmbot llvmbot added the lldb label Apr 25, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 25, 2025

@llvm/pr-subscribers-lldb

Author: David Spickett (DavidSpickett)

Changes

The new test added in #132783 was failing on Windows because it created a new error to say it did not support the feature, but then returned the existing, default constructed error. Which was a success value.

This also changes the GDBRemote error message to the same phrasing used in all the other places so we don't have to special case any platform.


Full diff: https://github.com/llvm/llvm-project/pull/137351.diff

3 Files Affected:

  • (modified) lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp (+3-3)
  • (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+1-1)
  • (modified) lldb/test/API/commands/process/reverse-continue/TestReverseContinueNotSupported.py (+3-2)
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index 9196ac2702c85..331139d029f50 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -239,15 +239,15 @@ ProcessWindows::DoAttachToProcessWithID(lldb::pid_t pid,
 Status ProcessWindows::DoResume(RunDirection direction) {
   Log *log = GetLog(WindowsLog::Process);
   llvm::sys::ScopedLock lock(m_mutex);
-  Status error;
 
   if (direction == RunDirection::eRunReverse) {
-    error.FromErrorStringWithFormatv(
+    return Status::FromErrorStringWithFormatv(
         "error: {0} does not support reverse execution of processes",
         GetPluginName());
-    return error;
   }
 
+  Status error;
+
   StateType private_state = GetPrivateState();
   if (private_state == eStateStopped || private_state == eStateCrashed) {
     LLDB_LOG(log, "process {0} is in state {1}.  Resuming...",
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index b616e99be83b2..f924d1194e8a1 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1410,7 +1410,7 @@ Status ProcessGDBRemote::DoResume(RunDirection direction) {
           LLDB_LOGF(log, "ProcessGDBRemote::DoResume: target does not "
                          "support reverse-continue");
           return Status::FromErrorString(
-              "target does not support reverse-continue");
+              "target does not support reverse execution of processes");
         }
 
         if (num_continue_C_tids > 0) {
diff --git a/lldb/test/API/commands/process/reverse-continue/TestReverseContinueNotSupported.py b/lldb/test/API/commands/process/reverse-continue/TestReverseContinueNotSupported.py
index 54d757dc4b98b..167d5827bff55 100644
--- a/lldb/test/API/commands/process/reverse-continue/TestReverseContinueNotSupported.py
+++ b/lldb/test/API/commands/process/reverse-continue/TestReverseContinueNotSupported.py
@@ -11,7 +11,6 @@
 
 
 class TestReverseContinueNotSupported(TestBase):
-    @skipIfWindows
     def test_reverse_continue_not_supported(self):
         target = self.connect()
 
@@ -26,7 +25,9 @@ def test_reverse_continue_not_supported(self):
         self.expect(
             "process continue --reverse",
             error=True,
-            substrs=["target does not support reverse-continue"],
+            # This error is "<plugin name> does not support...". The plugin name changes
+            # between platforms.
+            substrs=["does not support reverse execution of processes"],
         )
 
     def test_reverse_continue_forward_and_reverse(self):

@DavidSpickett
Copy link
Collaborator Author

@rocallahan FYI.

@DavidSpickett DavidSpickett merged commit 332e181 into llvm:main Apr 25, 2025
10 of 11 checks passed
@DavidSpickett DavidSpickett deleted the lldb-winreverse branch April 25, 2025 17:04
@rocallahan
Copy link
Contributor

Thanks!!!

jyli0116 pushed a commit to jyli0116/llvm-project that referenced this pull request Apr 28, 2025
…lvm#137351)

The new test added in llvm#132783
was failing on Windows because it created a new error to say it did not
support the feature, but then returned the existing, default constructed
error. Which was a success value.

This also changes the GDBRemote error message to the same phrasing used
in all the other places so we don't have to special case any platform.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…lvm#137351)

The new test added in llvm#132783
was failing on Windows because it created a new error to say it did not
support the feature, but then returned the existing, default constructed
error. Which was a success value.

This also changes the GDBRemote error message to the same phrasing used
in all the other places so we don't have to special case any platform.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…lvm#137351)

The new test added in llvm#132783
was failing on Windows because it created a new error to say it did not
support the feature, but then returned the existing, default constructed
error. Which was a success value.

This also changes the GDBRemote error message to the same phrasing used
in all the other places so we don't have to special case any platform.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…lvm#137351)

The new test added in llvm#132783
was failing on Windows because it created a new error to say it did not
support the feature, but then returned the existing, default constructed
error. Which was a success value.

This also changes the GDBRemote error message to the same phrasing used
in all the other places so we don't have to special case any platform.
Ankur-0429 pushed a commit to Ankur-0429/llvm-project that referenced this pull request May 9, 2025
…lvm#137351)

The new test added in llvm#132783
was failing on Windows because it created a new error to say it did not
support the feature, but then returned the existing, default constructed
error. Which was a success value.

This also changes the GDBRemote error message to the same phrasing used
in all the other places so we don't have to special case any platform.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants