Skip to content

debuginfo: Add script for Rust support in lldb-mi #89163

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

Closed

Conversation

YakoYakoYokuYoku
Copy link

As of the merge of PR lldb-tools/lldb-mi#80 made possible for lldb-mi to run a command before execution this PR extends Rust debugging with the tool.

Tested via launching a debugging session with the C/C++ VSCode extension and setting the miDebuggerPath field to /usr/bin/rust-lldb-mi.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 22, 2021
@Mark-Simulacrum
Copy link
Member

At a glance, I'd guess that this is not enough for the script to get shipped with Rust -- it probably needs to get added somewhere around here.

Additionally, I haven't heard of lldb-mi before -- can you share more details about whether it's an official component of the upstream lldb project? I'm not sure we should be shipping a wrapper for it otherwise...

cc @rust-lang/wg-llvm in case you have useful context

@nagisa
Copy link
Member

nagisa commented Sep 30, 2021

It seems like it used to be a part of the lldb project, but not anymore.

@Mark-Simulacrum
Copy link
Member

I'll r? @cuviper on this (as a member of wg-llvm as well as someone with distro experience), since I don't feel prepared to just merge this. I don't think we have any policy on which debuggers we ship "shims" for -- historically, it's just been lldb and gdb, which are the most prominent ones.

Ideally I think we'd want to avoid an ever increasing set of such scripts, particularly with no tests in tree, since it's very easy for them to unknowingly go stale (and that seems like a subpar experience). It looks like lldb-mi is shipped in the standard Ubuntu packages, so that's a potential "minimum" for inclusion, and may ease us testing it in CI to at least some level.

Curious to hear what others think though.

@cuviper
Copy link
Member

cuviper commented Oct 9, 2021

Additionally, I haven't heard of lldb-mi before -- can you share more details about whether it's an official component of the upstream lldb project? I'm not sure we should be shipping a wrapper for it otherwise...

I guess it's been answered now that this was split from llvm-project, and it looks like the lldb-tools org only has one member. So it doesn't look very official anymore, but there are still a handful of contributors working on it.

I'm still not clear what it is though -- something similar to gdbserver, perhaps?

It looks like lldb-mi is shipped in the standard Ubuntu packages,

I don't think it is. In 21.04, the lldb package has a broken symlink /usr/bin/lldb-mi -> lldb-mi-12, while lldb-9 has a good /usr/bin/lldb-mi-9 -> ../lib/llvm-9/bin/lldb-mi. That would correspond with the dates on that mailing list announcement being shortly after 9 had branched for release. It looks like 21.10 has removed that broken symlink.

I'm mildly inclined to let this merge just for src/etc/, but not to add it to dist, set rustup shims, etc.

@Mark-Simulacrum
Copy link
Member

Ah, I had just looked at the dpkg output I think, rather than the unpacked files.

I don't personally think there's much value (and some cost) to adding src/etc files that likely won't really be used by anyone - perhaps the file would be better placed in lldb-mi upstream in this case?

@cuviper
Copy link
Member

cuviper commented Oct 9, 2021

As for what, I think it's an implementation of GDB/MI, which you could use for embedding lldb in other tools that expect gdb.

@YakoYakoYokuYoku
Copy link
Author

Much of the purpose of this PR was to provide a GDB/MI LLDB interface for Rust debugging with tools that use MI (e.g. the C/C++ tools for VSCode) for situations that may require an alternative like issue #85267. As for merging this I have no problems with doing a PR to the lldb-mi repo, although I think integration can be done here via tests in-tree nonetheless.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021
@pnkfelix pnkfelix self-assigned this Oct 28, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2021
@pnkfelix
Copy link
Member

pnkfelix commented Jan 6, 2022

Thanks for putting up this Pull Request, @YakoYakoYokuYoku .

As others have already noted, we are not comfortable with adding more scripts to src/etc; at least, not scripts that don't have a stake-holder on the team.

We already are not doing a great job at supporting our "existing" debugger support, and I would prefer to first invest effort in fixing that before we even think about how to expand the scope of what debuggers we support in tree.

It sounds like you are willing to look into pushing this support into the lldb-mi repository instead. Which would be great. Maybe once that lands, we will be in a better place to look into adding the tests you suggest to our own repository. (Though I will just note that one reason that our debugger support is subpar is because of issues like #81813.)

@pnkfelix pnkfelix closed this Jan 6, 2022
@YakoYakoYokuYoku YakoYakoYokuYoku deleted the lldb-mi-script branch January 6, 2022 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants