[lldb] Use correct path for debugserver #131609
Conversation
|
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 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. |
|
@llvm/pr-subscribers-lldb Author: Yuval Deutscher (yuvald-sweet-security) ChangesHello, This solves an issue that arises when running lldb-server through a symlink which is not named exactly It turns out that there is a cascade of bugs here:
Given the above information, the most logical solution seems to be just ditching Full diff: https://github.com/llvm/llvm-project/pull/131609.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index dad72a176b5fa..e754d01be3629 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -906,7 +906,7 @@ FileSpec GDBRemoteCommunication::GetDebugserverPath(Platform *platform) {
FileSystem::Instance().Exists(debugserver_file_spec);
if (!debugserver_exists) {
// The debugserver binary is in the LLDB.framework/Resources directory.
- debugserver_file_spec = HostInfo::GetSupportExeDir();
+ debugserver_file_spec.SetDirectory(HostInfo::GetProgramFileSpec().GetDirectory());
if (debugserver_file_spec) {
debugserver_file_spec.AppendPathComponent(DEBUGSERVER_BASENAME);
debugserver_exists = FileSystem::Instance().Exists(debugserver_file_spec);
|
|
Well, looks like the tests really do check a case where lldb and lldb-server are not in the same directory. Should we just try both paths? maybe make EDIT: trying a new approach |
yuvald-sweet-security
force-pushed
the
fix-lldb-debugserver-path
branch
from
f47298e to
a4e2c9f
Compare
|
Solves #63468 |
|
@labath now that we've fixed the other issue, can you weigh in on my solution for this one? If you don't have time and want to deal with this next week, that's also fine, just making sure this PR didn't get forgotten |
|
Long term, I think we should have a different way to set/compute/retrieve these paths, but locally, what you're doing is correct. lldb-server doesn't have a way to compute the "shared library directory", nor does it have any reason to do that, so returning an empty file spec instead of a bogus value is definitely an improvement. I'm just thinking about whether there's a way to test this. You've said that there's a test for the scenario where lldb-server and lldb are not in the same directory. Which one is it? Would it be possible to use a similar approach to test this scenario as well? |
I think all the tests for LLDB Python API indirectly test this code, when I tried my initial approach (always relying on I'm assuming this is because the python executable used to execute the test code is not in the same directory with lldb-server. But I'm not sure if this helps our cause if what you mean is that you want a test that tests the exact scenario in the description, i.e. running lldb-server from a symlink and seeing that it correctly finds itself as a gdb-server |
|
Yea, these tests only test the Do you know if there are any tests that run lldb-server? I couldn't find anything by grepping the repo |
|
|
hm, seems useful. do you think it makes sense to just modify it to run through a symlink, e.g. or should I duplicate it and create a new test? |
|
Given how fiddly this all is, I would duplicate the test (meaning, add another test case to that file). Even if there is overlap, it's worth it for the added clarity. |
yuvald-sweet-security
force-pushed
the
fix-lldb-debugserver-path
branch
2 times, most recently
from
1b04782 to
bb04159
Compare
|
Wrote a test and it seems to pass CI, what do you say? |
|
I think it's a good start (thanks for helping out, David), but I don't get the removal part. What you actually want to test that it succeeds in launching the llgs persona, is it not? Wouldn't it be better to not remove the lldb-server and check that the "run" command actually succeeds? (you can check that the debugged binary returns the correct exit code, stops at some breakpoint or whatever is easiest) |
|
Whoops, I copied that test and didn't read what it does at the end |
yuvald-sweet-security
force-pushed
the
fix-lldb-debugserver-path
branch
2 times, most recently
from
c2fb1da to
fcc85b8
Compare
|
Should be good now |
| new_lldb_server = os.path.join(llgs_hiding_directory, "lldb-server") | ||
| os.makedirs(llgs_hiding_directory) | ||
| shutil.copy(lldbgdbserverutils.get_lldb_server_exe(), new_lldb_server) |
There was a problem hiding this comment.
Do you need to make the copy if you're not going to delete it?
There was a problem hiding this comment.
that's an excellent question
| connect_url = "connect://[%s]:%s" % (hostname, socket_id) | ||
| self.runCmd("platform connect %s" % connect_url) | ||
| self.runCmd("target create {}".format(self.getBuildArtifact("a.out"))) | ||
| self.runCmd("run") |
There was a problem hiding this comment.
I would feel better if there was some positive acknowledgement that the process was running, rather than just a lack of error. Would adding something like
self.expect(
"process status",
patterns=["Process .* exited with status = 0"],
)
work?
| # So that lldb-server doesn't crash over SIGHUP | ||
| os.kill(proc.pid, signal.SIGTERM) |
There was a problem hiding this comment.
I'm surprised that this is necessary. spawnSubprocess should arrange for the process to be killed at the end of the test. What exactly happens if you remove this?
There was a problem hiding this comment.
The test still passes, but you get a weird error about lldb-server crashing on SIGHUP (another bug? see below). This also happens in the other test in that file, but I didn't bother with it, I fixed it for the new test because it threw me off when debugging it.
SIGHUP received, exiting lldb-server...
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0. Program arguments: /home/user/random/llvm-project/build/lldb-test-build.noindex/commands/platform/launchgdbserver/TestPlatformLaunchGDBServer.test_platform_process_launch_gdb_server/lldb-server platform --listen [127.0.0.1]:0 --socket-file /home/user/random/llvm-project/build/lldb-test-build.noindex/commands/platform/launchgdbserver/TestPlatformLaunchGDBServer.test_platform_process_launch_gdb_server/port -- /home/user/random/llvm-project/build/lldb-test-build.noindex/commands/platform/launchgdbserver/TestPlatformLaunchGDBServer.test_platform_process_launch_gdb_server/a.out foo
#0 0x000062736b6aea2d (+0x3f3ca2d)
#1 0x000062736b6aeeeb (+0x3f3ceeb)
#2 0x000062736b6ad08f (+0x3f3b08f)
#3 0x000062736b6af5c9 (+0x3f3d5c9)
#4 0x0000781b8f245330 (/lib/x86_64-linux-gnu/libc.so.6+0x45330)
#5 0x0000781b8f29eb2c pthread_kill (/lib/x86_64-linux-gnu/libc.so.6+0x9eb2c)
#6 0x0000781b8f24527e raise (/lib/x86_64-linux-gnu/libc.so.6+0x4527e)
#7 0x0000781b8f2288ff abort (/lib/x86_64-linux-gnu/libc.so.6+0x288ff)
#8 0x000062736b56ca9e (+0x3dfaa9e)
#9 0x0000781b8f245330 (/lib/x86_64-linux-gnu/libc.so.6+0x45330)
#10 0x0000781b8f31ba00 ppoll (/lib/x86_64-linux-gnu/libc.so.6+0x11ba00)
#11 0x000062736b717281 (+0x3fa5281)
#12 0x000062736b717193 (+0x3fa5193)
#13 0x000062736b717fe8 (+0x3fa5fe8)
#14 0x000062736b56c984 (+0x3dfa984)
#15 0x000062736b5774fb (+0x3e054fb)
#16 0x0000781b8f22a1ca (/lib/x86_64-linux-gnu/libc.so.6+0x2a1ca)
#17 0x0000781b8f22a28b __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28b)
#18 0x000062736b55b6e5 (+0x3de96e5)
There was a problem hiding this comment.
Yea, this seems to happen also on the lldb-server-20 I got from apt, outside of the test...
$ lldb-server-20 platform --server --listen '*:1338' --log-channels "lldb all" &
[1] 225430
Listen to [*]:1338
Listen to [*]:0
$ kill -HUP `pidof lldb-server-20`
SIGHUP received, exiting lldb-server...
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0. Program arguments: % lldb-server-20 platform --server --listen *:1338 --log-channels "lldb all"
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0 libLLVM.so.20.1 0x000074b07842bc7f llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 63
1 libLLVM.so.20.1 0x000074b078429989 llvm::sys::RunSignalHandlers() + 89
2 libLLVM.so.20.1 0x000074b07842c390
3 libc.so.6 0x000074b076c45330
4 libc.so.6 0x000074b076c9eb2c pthread_kill + 284
5 libc.so.6 0x000074b076c4527e gsignal + 30
6 libc.so.6 0x000074b076c288ff abort + 223
7 lldb-server-20 0x00005d1b74ca3b57
8 libc.so.6 0x000074b076c45330
9 libc.so.6 0x000074b076d1b9ad ppoll + 77
10 lldb-server-20 0x00005d1b74cc2eea
11 lldb-server-20 0x00005d1b74cc3dff
12 lldb-server-20 0x00005d1b74ca39a1
13 lldb-server-20 0x00005d1b74ca6225
14 libc.so.6 0x000074b076c2a1ca
15 libc.so.6 0x000074b076c2a28b __libc_start_main + 139
16 lldb-server-20 0x00005d1b74c9e055
[1] + 225430 IOT instruction (core dumped) lldb-server-20 platform --server --listen '*:1338' --log-channels "lldb all"
whats the intended behavior here, do we expect lldb-server to handle SIGHUP without crashing or is the issue in the test sending that signal?
There was a problem hiding this comment.
Okay, yes, I think I've seen this error as well. It probably is another bug (in lldb-server, or in the test suite). If it does not impact the result of the test, let's remove this, as this should be handled generally.
There was a problem hiding this comment.
Alright, done.
yuvald-sweet-security
force-pushed
the
fix-lldb-debugserver-path
branch
from
fcc85b8 to
1efdb93
Compare
yuvald-sweet-security
force-pushed
the
fix-lldb-debugserver-path
branch
from
1efdb93 to
67b33af
Compare
| @@ -4,6 +4,7 @@ | |||
| """ | |||
|
|
|||
| import os | |||
| import signal | |||
There was a problem hiding this comment.
I guess this isn't used anymore.
yuvald-sweet-security
force-pushed
the
fix-lldb-debugserver-path
branch
from
67b33af to
e6bd01e
Compare
|
@JDevlieghere can you merge this? I don't have merge access. Also, do you think this is worth backporting to 20.x? |
|
I can push the button for you if you could just update the patch description to what you'd like the commit message to say (in particular, I think the last paragraph is no longer correct). As for cherry-picking, is this related to the lldb-server platform refactor in any way, or is it a separate problem that has always been there (I'm trying to figure out if this is a regression, at least for some cases) |
Alright, I edited it. This is not related to the lldb-server platform refactor, I get this issue even in Ubuntu's packaged lldb-server-19. |
|
@yuvald-sweet-security Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Got it. In that case, I'd probably go with not cherry-picking this since it's not a regression. |
|
FYI, I reverted this because of problems test failures on mac: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/24395/testReport/junit/lldb-api/commands_target_auto-install-main-executable/TestAutoInstallMainExecutable_py/ I'm not quite sure what's happening, but it seems pretty clear that lldb is failing to find the debugserver binary (on mac, the gdbserver is in a different binary). If you have access to a mac, you should be able to reproduce it yourself (it worked for me, even though I often get different results from the green dragon). If not, maybe we can ask one of the apple folks to take a look. That said, inspired by your last PR (which, unsurprisingly, broke some of our symlink configurations), I'm going to be refactoring the code which launches debug servers soon(ish), and I think I can make it work with your setup as well. So, if you're not in particular hurry, you may not need to do anything. |
Don't have a mac at hand here but I do have one in another place (tagging myself - @hadeutscher). I might get to fixing this next week. |
This solves an issue that arises when running lldb-server through a symlink which is not named exactly `lldb-server`. For example, in many distros lldb-server is packaged as e.g. `/usr/lib/llvm-19/bin/lldb-server` which is then accessed through a symlink such as `/usr/bin/lldb-server-19`. It turns out that there is a cascade of bugs here: * `GetShlibDir` attempts to locate the LLVM library directory by calling `GetModuleFileSpecForHostAddress` on the address of the function `ComputeSharedLibraryDirectory`, assuming that it is inside `liblldb.so`. However, in every packaging I've seen of lldb-server the function `ComputeSharedLibraryDirectory` is statically linked into the `lldb-server` binary and is not in `liblldb.so`. * When run through a symlink, `GetModuleFileSpecForHostAddress` on an address that is in `lldb-server` returns the path of the symlink, not the path of the binary itself. So we get e.g. `/usr/bin/` and not `/usr/lib/llvm-19/bin/`. * `GetDebugserverPath` attempts to concat `"lldb-server"` to the directory we obtained, and thus fails when the symlink is not named exactly `lldb-server`. * Ironically, the reason that this works in the first place is precisely because `GetModuleFileSpecForHostAddress` returns an incorrect path - when the server is run as `lldb-server-19 ...` it returns `"lldb-server-19"` which then causes `ComputePathRelativeToLibrary` to fail and then `ComputeSupportExeDirectory` falls back to just using `GetProgramFileSpec` instead (which is the only option that actually yields a correct path).
This solves an issue that arises when running lldb-server through a symlink which is not named exactly `lldb-server`. For example, in many distros lldb-server is packaged as e.g. `/usr/lib/llvm-19/bin/lldb-server` which is then accessed through a symlink such as `/usr/bin/lldb-server-19`. It turns out that there is a cascade of bugs here: * `GetShlibDir` attempts to locate the LLVM library directory by calling `GetModuleFileSpecForHostAddress` on the address of the function `ComputeSharedLibraryDirectory`, assuming that it is inside `liblldb.so`. However, in every packaging I've seen of lldb-server the function `ComputeSharedLibraryDirectory` is statically linked into the `lldb-server` binary and is not in `liblldb.so`. * When run through a symlink, `GetModuleFileSpecForHostAddress` on an address that is in `lldb-server` returns the path of the symlink, not the path of the binary itself. So we get e.g. `/usr/bin/` and not `/usr/lib/llvm-19/bin/`. * `GetDebugserverPath` attempts to concat `"lldb-server"` to the directory we obtained, and thus fails when the symlink is not named exactly `lldb-server`. * Ironically, the reason that this works in the first place is precisely because `GetModuleFileSpecForHostAddress` returns an incorrect path - when the server is run as `lldb-server-19 ...` it returns `"lldb-server-19"` which then causes `ComputePathRelativeToLibrary` to fail and then `ComputeSupportExeDirectory` falls back to just using `GetProgramFileSpec` instead (which is the only option that actually yields a correct path).
This solves an issue that arises when running lldb-server through a symlink which is not named exactly `lldb-server`. For example, in many distros lldb-server is packaged as e.g. `/usr/lib/llvm-19/bin/lldb-server` which is then accessed through a symlink such as `/usr/bin/lldb-server-19`. It turns out that there is a cascade of bugs here: * `GetShlibDir` attempts to locate the LLVM library directory by calling `GetModuleFileSpecForHostAddress` on the address of the function `ComputeSharedLibraryDirectory`, assuming that it is inside `liblldb.so`. However, in every packaging I've seen of lldb-server the function `ComputeSharedLibraryDirectory` is statically linked into the `lldb-server` binary and is not in `liblldb.so`. * When run through a symlink, `GetModuleFileSpecForHostAddress` on an address that is in `lldb-server` returns the path of the symlink, not the path of the binary itself. So we get e.g. `/usr/bin/` and not `/usr/lib/llvm-19/bin/`. * `GetDebugserverPath` attempts to concat `"lldb-server"` to the directory we obtained, and thus fails when the symlink is not named exactly `lldb-server`. * Ironically, the reason that this works in the first place is precisely because `GetModuleFileSpecForHostAddress` returns an incorrect path - when the server is run as `lldb-server-19 ...` it returns `"lldb-server-19"` which then causes `ComputePathRelativeToLibrary` to fail and then `ComputeSupportExeDirectory` falls back to just using `GetProgramFileSpec` instead (which is the only option that actually yields a correct path).
This solves an issue that arises when running lldb-server through a symlink which is not named exactly
lldb-server. For example, in many distros lldb-server is packaged as e.g./usr/lib/llvm-19/bin/lldb-serverwhich is then accessed through a symlink such as/usr/bin/lldb-server-19.It turns out that there is a cascade of bugs here:
GetShlibDirattempts to locate the LLVM library directory by callingGetModuleFileSpecForHostAddresson the address of the functionComputeSharedLibraryDirectory, assuming that it is insideliblldb.so. However, in every packaging I've seen of lldb-server the functionComputeSharedLibraryDirectoryis statically linked into thelldb-serverbinary and is not inliblldb.so.GetModuleFileSpecForHostAddresson an address that is inlldb-serverreturns the path of the symlink, not the path of the binary itself. So we get e.g./usr/bin/and not/usr/lib/llvm-19/bin/.GetDebugserverPathattempts to concat"lldb-server"to the directory we obtained, and thus fails when the symlink is not named exactlylldb-server.GetModuleFileSpecForHostAddressreturns an incorrect path - when the server is run aslldb-server-19 ...it returns"lldb-server-19"which then causesComputePathRelativeToLibraryto fail and thenComputeSupportExeDirectoryfalls back to just usingGetProgramFileSpecinstead (which is the only option that actually yields a correct path).