Skip to content

[lldb] Correctly invalidate unloaded image tokens #65945

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
Sep 11, 2023

Conversation

DavidSpickett
Copy link
Collaborator

Some functions in Process were using LLDB_INVALID_ADDRESS instead of LLDB_INVALID_TOKEN.

The only visible effect of this appears to be that "process unload " would complete to 0 even after the image was unloaded. Since the command is checking for LLDB_INVALID_TOKEN.

Everything else worked somehow. I've added a check to the existing load unload tests anyway.

The tab completion cannot be checked as is, but when I make them more strict in a later patch it will be tested.

@DavidSpickett DavidSpickett requested a review from a team as a code owner September 11, 2023 10:52
@llvmbot llvmbot added the lldb label Sep 11, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 11, 2023

@llvm/pr-subscribers-lldb

Changes

Some functions in Process were using LLDB_INVALID_ADDRESS instead of LLDB_INVALID_TOKEN.

The only visible effect of this appears to be that "process unload " would complete to 0 even after the image was unloaded. Since the command is checking for LLDB_INVALID_TOKEN.

Everything else worked somehow. I've added a check to the existing load unload tests anyway.

The tab completion cannot be checked as is, but when I make them more strict in a later patch it will be tested.

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

4 Files Affected:

  • (modified) lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp (+1-1)
  • (modified) lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp (+1-1)
  • (modified) lldb/source/Target/Process.cpp (+2-2)
  • (modified) lldb/test/API/functionalities/load_unload/TestLoadUnload.py (+9)
diff --git a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
index 1e91f2ccd198259..b4f1b76c39dbebf 100644
--- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -949,7 +949,7 @@ uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process,
 Status PlatformPOSIX::UnloadImage(lldb_private::Process *process,
                                   uint32_t image_token) {
   const addr_t image_addr = process->GetImagePtrFromToken(image_token);
-  if (image_addr == LLDB_INVALID_ADDRESS)
+  if (image_addr == LLDB_INVALID_IMAGE_TOKEN)
     return Status("Invalid image token");
 
   StreamString expr;
diff --git a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
index cbac14e2ccf7a92..88d543289a8470e 100644
--- a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -415,7 +415,7 @@ uint32_t PlatformWindows::DoLoadImage(Process *process,
 
 Status PlatformWindows::UnloadImage(Process *process, uint32_t image_token) {
   const addr_t address = process->GetImagePtrFromToken(image_token);
-  if (address == LLDB_INVALID_ADDRESS)
+  if (address == LLDB_INVALID_IMAGE_TOKEN)
     return Status("invalid image token");
 
   StreamString expression;
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 2b0774588138881..f82ab05362fbee9 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -5911,12 +5911,12 @@ size_t Process::AddImageToken(lldb::addr_t image_ptr) {
 lldb::addr_t Process::GetImagePtrFromToken(size_t token) const {
   if (token < m_image_tokens.size())
     return m_image_tokens[token];
-  return LLDB_INVALID_ADDRESS;
+  return LLDB_INVALID_IMAGE_TOKEN;
 }
 
 void Process::ResetImageToken(size_t token) {
   if (token < m_image_tokens.size())
-    m_image_tokens[token] = LLDB_INVALID_ADDRESS;
+    m_image_tokens[token] = LLDB_INVALID_IMAGE_TOKEN;
 }
 
 Address
diff --git a/lldb/test/API/functionalities/load_unload/TestLoadUnload.py b/lldb/test/API/functionalities/load_unload/TestLoadUnload.py
index 7e8acfa3021acfc..2208e520f1d5612 100644
--- a/lldb/test/API/functionalities/load_unload/TestLoadUnload.py
+++ b/lldb/test/API/functionalities/load_unload/TestLoadUnload.py
@@ -307,6 +307,15 @@ def run_lldb_process_load_and_unload_commands(self):
             patterns=["Unloading .* with index %s.*ok" % index],
         )
 
+        # Confirm that we unloaded properly.
+        self.expect(
+            "image lookup -n a_function",
+            "a_function should not exist after unload",
+            error=True,
+            matching=False,
+            patterns=["1 match found"],
+        )
+
         self.runCmd("process continue")
 
     @expectedFailureAll(oslist=["windows"])  # breakpoint not hit

Some functions in Process were using LLDB_INVALID_ADDRESS
instead of LLDB_INVALID_TOKEN.

The only visible effect of this appears to be that
"process unload <tab>" would complete to 0 even after the
image was unloaded. Since the command is checking for
LLDB_INVALID_TOKEN.

Everything else worked somehow. I've added a check to the
existing load unload tests anyway.

The tab completion cannot be checked as is, but when I make
them more strict in a later patch it will be tested.
@DavidSpickett DavidSpickett merged commit 3125bd4 into llvm:main Sep 11, 2023
@DavidSpickett DavidSpickett deleted the lldb-image-token branch September 11, 2023 16:12
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
Some functions in Process were using LLDB_INVALID_ADDRESS instead of
LLDB_INVALID_TOKEN.

The only visible effect of this appears to be that "process unload
<tab>" would complete to 0 even after the image was unloaded. Since the
command is checking for LLDB_INVALID_TOKEN.

Everything else worked somehow. I've added a check to the existing load
unload tests anyway.

The tab completion cannot be checked as is, but when I make them more
strict in a later patch it will be tested.
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.

3 participants