Skip to content

Conversation

@talkeren
Copy link

@talkeren talkeren commented May 19, 2025

If MachProcess::GetProcessPlatformViaDYLDSPI fails and return 0, we don't
want to call it everytime as it won't change the result. So use
std::optional to cache wether we already calculated the value.

Alleviate the impact of issue #140610

@talkeren talkeren requested a review from JDevlieghere as a code owner May 19, 2025 20:17
@github-actions
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the lldb label May 19, 2025
@talkeren talkeren force-pushed the get-platform-cache branch from 788597d to 7ccef56 Compare May 19, 2025 20:17
@llvmbot
Copy link
Member

llvmbot commented May 19, 2025

@llvm/pr-subscribers-lldb

Author: Tal Keren (talkeren)

Changes

If MachProcess::GetProcessPlatformViaDYLDSPI fails and return 0, we don't
want to call it everytime as it won't change the result. So use
std::optional to cache wether we already calculated the value.


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

2 Files Affected:

  • (modified) lldb/tools/debugserver/source/MacOSX/MachProcess.h (+1-1)
  • (modified) lldb/tools/debugserver/source/MacOSX/MachProcess.mm (+7-5)
diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.h b/lldb/tools/debugserver/source/MacOSX/MachProcess.h
index 56bc9d6c7461e..516a77c0bd6c8 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachProcess.h
+++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.h
@@ -398,7 +398,7 @@ class MachProcess {
 
   pid_t m_pid;           // Process ID of child process
   cpu_type_t m_cpu_type; // The CPU type of this process
-  uint32_t m_platform;   // The platform of this process
+  std::optional<uint32_t> m_platform;   // The platform of this process
   int m_child_stdin;
   int m_child_stdout;
   int m_child_stderr;
diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
index 3afaaa2f64c00..ce74dd1cadfb0 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
+++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
@@ -1039,9 +1039,9 @@ static bool mach_header_validity_test(uint32_t magic, uint32_t cputype) {
 };
 
 uint32_t MachProcess::GetPlatform() {
-  if (m_platform == 0)
+  if (!m_platform)
     m_platform = MachProcess::GetProcessPlatformViaDYLDSPI();
-  return m_platform;
+  return *m_platform;
 }
 
 uint32_t MachProcess::GetProcessPlatformViaDYLDSPI() {
@@ -1058,7 +1058,9 @@ static bool mach_header_validity_test(uint32_t magic, uint32_t cputype) {
   }
   return platform;
 }
-
+If `MachProcess::GetProcessPlatformViaDYLDSPI` fails and return 0, we don't
+want to call it everytime as it won't change the result. So use
+std::optional to cache wether we already calculated the value.
 void MachProcess::GetAllLoadedBinariesViaDYLDSPI(
     std::vector<struct binary_image_information> &image_infos) {
   kern_return_t kern_ret;
@@ -1323,7 +1325,7 @@ static bool mach_header_validity_test(uint32_t magic, uint32_t cputype) {
   // Clear any cached thread list while the pid and task are still valid
 
   m_task.Clear();
-  m_platform = 0;
+  m_platform.reset();
   // Now clear out all member variables
   m_pid = INVALID_NUB_PROCESS;
   if (!detaching)
@@ -1754,7 +1756,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
 
   // NULL our task out as we have already restored all exception ports
   m_task.Clear();
-  m_platform = 0;
+  m_platform.reset();
 
   // Clear out any notion of the process we once were
   const bool detaching = true;



If `MachProcess::GetProcessPlatformViaDYLDSPI` fails and return 0, we don't want to call it everytime as it won't change the result. So use std::optional to cache wether we already calculated the value.
@talkeren talkeren force-pushed the get-platform-cache branch from 7ccef56 to c385e97 Compare May 19, 2025 20:18
@JDevlieghere JDevlieghere requested a review from jasonmolenda May 19, 2025 20:21
@talkeren talkeren changed the title [lldb] Properly cache the result of MachProcess::GetPlatform #140610 [lldb] Properly cache the result of MachProcess::GetPlatform May 19, 2025
@github-actions
Copy link

github-actions bot commented May 19, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd like @jasonmolenda to sign off as the debugserver maintainer.

@talkeren
Copy link
Author

Closing it following the discussion in the issue - I made a new PR that fixes it differently without change the current behaviour.

@talkeren talkeren closed this May 21, 2025
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