-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[lldb] Fix FindProcessImpl() for iOS simulators #139174
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
[lldb] Fix FindProcessImpl() for iOS simulators #139174
Conversation
@llvm/pr-subscribers-lldb Author: None (royitaqi) ChangesBenefitThis patch fixes:
Changes
Manual testWith this patch:
Without this patch:
UnittestI did a couple of code searches (1, 2) and didn't seem to find any existing tests. Any suggestion about how to test this? Full diff: https://github.com/llvm/llvm-project/pull/139174.diff 1 Files Affected:
diff --git a/lldb/source/Host/macosx/objcxx/Host.mm b/lldb/source/Host/macosx/objcxx/Host.mm
index e187bf98188ae..e8a1c597eea53 100644
--- a/lldb/source/Host/macosx/objcxx/Host.mm
+++ b/lldb/source/Host/macosx/objcxx/Host.mm
@@ -595,7 +595,9 @@ DataExtractor data(arg_data.GetBytes(), arg_data_size,
const llvm::Triple::ArchType triple_arch = triple.getArch();
const bool check_for_ios_simulator =
(triple_arch == llvm::Triple::x86 ||
- triple_arch == llvm::Triple::x86_64);
+ triple_arch == llvm::Triple::x86_64 ||
+ triple_arch == llvm::Triple::aarch64);
+
const char *cstr = data.GetCStr(&offset);
if (cstr) {
process_info.GetExecutableFile().SetFile(cstr, FileSpec::Style::native);
@@ -621,22 +623,25 @@ DataExtractor data(arg_data.GetBytes(), arg_data_size,
}
Environment &proc_env = process_info.GetEnvironment();
+ bool is_simulator = false;
while ((cstr = data.GetCStr(&offset))) {
if (cstr[0] == '\0')
break;
- if (check_for_ios_simulator) {
- if (strncmp(cstr, "SIMULATOR_UDID=", strlen("SIMULATOR_UDID=")) ==
- 0)
- process_info.GetArchitecture().GetTriple().setOS(
- llvm::Triple::IOS);
- else
- process_info.GetArchitecture().GetTriple().setOS(
- llvm::Triple::MacOSX);
- }
+ if (check_for_ios_simulator &&
+ strncmp(cstr, "SIMULATOR_UDID=", strlen("SIMULATOR_UDID=")) ==
+ 0)
+ is_simulator = true;
proc_env.insert(cstr);
}
+ llvm::Triple &triple = process_info.GetArchitecture().GetTriple();
+ if (is_simulator) {
+ triple.setOS(llvm::Triple::IOS);
+ triple.setEnvironment(llvm::Triple::Simulator);
+ } else {
+ triple.setOS(llvm::Triple::MacOSX);
+ }
return true;
}
}
|
Could we test this in |
@JDevlieghere Thanks for pointer. It seems the tests in that file are all skipped because of this bug number: E.g.
Did a bit internet search and couldn't find how to find more info about this bug or why these tests are all skipped. Not sure if I should un-skip them. What's your advice on my next steps? |
Ha, that's my radar, and it's no longer relevant. I bisected an issue with the test suite to that particular test, but the last comment says that it wasn't the culprit after all, so there's no reason it should still be disabled. |
Hi @JDevlieghere, I don't think the test file was very happy when I removed that skip statement ( When you have the time, I wonder if you could kindly try on your side and see if they work for you. FWIW I'm using the following command to build and test:
|
Thanks for the heads up. The build issue was because we didn't pass |
Hi @JDevlieghere , Thank you very much for your #142244. I have rebased this patch on top of that and added assertions into the iOS test case to verify that we can get a list of processes on the iOS simulator. There is something that I don't think blocks this PR but would like to understand:
Thanks, |
✅ With the latest revision this PR passed the Python code formatter. |
I can definitely see the binary running:
If it's not showing up in the |
Hi @JDevlieghere, I think I know why - it's by design: In the Now, there are a few ways we can go:
Looking forward to your thoughts. Best, |
a8fbaca
to
771febc
Compare
Hi @JDevlieghere, when you have the time, could you kindly review / see my rely to your previous comment? |
Ah, that's unfortunate, but good catch. In
|
FWIW in general this is done so that you can have multiple debug sessions debugging the same binary being run multiple times. You run one lldb and attach to inferior-process(1), then you run it again, inferior-process(2), and want to attach to it from a second lldb. We don't want to show the inferior-process(1) as an attachable process at all, because only one debugger can control a process. So we only show inferior-process(2). I haven't followed along with the PR/code enough to know if that's applicable in this case, but that's the general thinking with lldb's process list algorithms - we are trying to show processes that you might be able to attach to. In debugserver well also filter out procsses that are zombies and such, iirc. |
Thanks for the context, @jasonmolenda . Yeah I kinda guessed that that's the reason. It's a good usability feature for sure. GJ to whoever implemented it~ --
@JDevlieghere Thanks for the suggestion~! Will look for it and update the test. - As always, I appreciate your suggestions, which have been leading me to learnings and better patches. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @JDevlieghere,
In TestAppleSimulatorOSType there's a snippet that launched a binary with simctl. Could we use that to launch the binary without it being traced?
Done. Kindly take a look when you have the time.
TL;DR changes:
- Found the snippet you were referring to in
TestAppleSimulatorOSType
. - Refactored two functions into
lldbutil
. - Used those two functions in
TestSimulatorPlatform
to launcha.out
separately and find it in the output ofplatform process list
(matching both pid and name). - In order to do 3, I had to rename
hello.c
and add code so that it prints the pid and also wait for 10s.
I have a question regarding 4, about how to make sure that the hello.cpp
builds in Windows. See inline comment.
7cac42a
to
be6580d
Compare
Hi @JDevlieghere and @clayborg , Could you kindly review when you have the chance? I think the PR is close to finish, except the "how to make sure hello.cpp builds in Windows" question. TL;DR updates:
Thanks, |
Does this test need to build on Windows? That test should only run on Darwin, so if it's properly skipped, we shouldn't even try to build the inferior. The Windows-conditional parts will be dead code. Besides that this LGTM |
Hi @JDevlieghere, I have removed the Windows related code. Should be good to go (re-running tests on the side). FWIW, my bad, I overlooked the |
Hi @clayborg, you want me to merge directly, or you want to take a final look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One possible nit about switching to using a while loop, but looks good.
# Benefit This patch fixes: 1. After `platform select ios-simulator`, `platform process list` will now print processes which are running in the iOS simulator. Previously, no process will be listed. 2. After `platform select ios-simulator`, `platform attach --name <name>` will succeed. Previously, it will error out saying no process is found. # Several bugs that is being fixed 1. During the process listing, add `aarch64` to the list of CPU types for which iOS simulators are checked for. 2. Given a candidate process, when checking for simulators, the original code will find the desired environment variable (`SIMULATOR_UDID`) and set the OS to iOS, but then the immediate next environment variable will set it back to macOS. 3. For processes running on simulator, set the triple's `Environment` to `Simulator`, so that such processes can pass the filtering [in this line](https://fburl.com/8nivnrjx). The original code leave it as the default `UnknownEnvironment`. # Manual test **With this patch:** ``` royshi-mac-home ~/public_llvm/build % bin/lldb (lldb) platform select ios-simulator (lldb) platform process list 240 matching processes were found on "ios-simulator" PID PARENT USER TRIPLE NAME ====== ====== ========== ============================== ============================ 40511 28844 royshi arm64-apple-ios-simulator FocusPlayground // my toy iOS app running on simulator ... // omit 28844 1 royshi arm64-apple-ios-simulator launchd_sim (lldb) process attach --name FocusPlayground Process 40511 stopped * thread llvm#1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP frame #0: 0x0000000104e3cb70 libsystem_kernel.dylib`mach_msg2_trap + 8 libsystem_kernel.dylib`mach_msg2_trap: -> 0x104e3cb70 <+8>: ret ... // omit ``` **Without this patch:** ``` $ bin/lldb (lldb) platform select ios-simulator (lldb) platform process list error: no processes were found on the "ios-simulator" platform (lldb) process attach --name FocusPlayground error: attach failed: could not find a process named FocusPlayground ``` # Unittest See PR.
Benefit
This patch fixes:
platform select ios-simulator
,platform process list
will now print processes which are running in the iOS simulator. Previously, no process will be listed.platform select ios-simulator
,platform attach --name <name>
will succeed. Previously, it will error out saying no process is found.Several bugs that is being fixed
aarch64
to the list of CPU types for which iOS simulators are checked for.SIMULATOR_UDID
) and set the OS to iOS, but then the immediate next environment variable will set it back to macOS.Environment
toSimulator
, so that such processes can pass the filtering in this line. The original code leave it as the defaultUnknownEnvironment
.Manual test
With this patch:
Without this patch:
Unittest
With the patch, the test can list processes on the iOS simulator:
Without the patch, the test cannot list processes on the iOS simulator: