Skip to content
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

Add lldb to the tools list #1492

Closed
wants to merge 1 commit into from
Closed

Add lldb to the tools list #1492

wants to merge 1 commit into from

Conversation

tromey
Copy link
Contributor

@tromey tromey commented Aug 29, 2018

No description provided.

@tromey
Copy link
Contributor Author

tromey commented Aug 29, 2018

This works in local testing on my macOS box.

I did not know how to write a test case for this, so I could use some help here.

@alexcrichton
Copy link
Member

Thanks for this! I'm a little hesitant to do this though as this is inserting a new lldb executable into users PATH which seems like it'll be easy to clash. For unstable tools like the LLVM binutils we only ship them in the sysroot and then rely on wrapper tools like cargo-binutils to find them, but is such a strategy not feasible for LLDB? If not, could this perhaps be called rust-lldb to avoid clashes with system-installed versions?

@tromey
Copy link
Contributor Author

tromey commented Aug 29, 2018

For unstable tools like the LLVM binutils we only ship them in the sysroot and then rely on wrapper tools like cargo-binutils to find them, but is such a strategy not feasible for LLDB?

I don't know, but I don't see why not for the command-line tool. I'm less certain about lldb-mi, which is probably only invoked by IDEs. Though maybe an IDE could be taught to say cargo lldb-mi or something like that.

If not, could this perhaps be called rust-lldb to avoid clashes with system-installed versions?

rust-lldb is already taken.

@tromey
Copy link
Contributor Author

tromey commented Aug 29, 2018

rust-lldb just invokes lldb from the path, so if we require cargo lldb or some such, then rust-lldb will have to be adapted somehow.

I tend to think installing an lldb in the user's path is the better approach, though. Primarily, I think, because you have to ask for this -- lldb isn't a default tool and it is only nightly.

@alexcrichton
Copy link
Member

Hm ok, well in any case I would personally be pretty uncomfortable hijacking the locally installed lldb, if any. I'm worried about confusing developers who aren't as familiar with rustup to know what's going on.

We could tweak rust-lldb yeah or simply repurpose it on OSX to the actual binary (which should probably have the pretty printers anyway)? Alternatively the script could be updated to search in the sysroot to find lldb shipped with the toolchain as well.

@tromey
Copy link
Contributor Author

tromey commented Sep 4, 2018

I'm worried about confusing developers who aren't as familiar with rustup to know what's going on

I'd like to take one last stab at convincing you that this is all ok. If you're still not convinced, I will look into the cargo binutils approach.

Here are the reasons I think it makes sense to make this link.

  1. Rust's lldb is optional. It is only available in nightly and it is not installed by default. Users must ask for it by adding a component. I think users are generally sophisticated enough to not need a second layer of safety -- the gate of asking for the package to be installed is enough.

  2. Rust's lldb is actually just the standard lldb. We just have a snapshot of upstream lldb and clang. It's easy to rebase these and bring over any fixes. Rust's lldb will work as well for C and C++ as any other lldb.

  3. If Rust's lldb is not installed, then the failure mode will be quite different: users will try lldb and it will not work in some situations. That will happen because only the Rust lldb will understand their program properly.

  4. Finally, this seems to be accepted practice in other toolchains. Swift has its own fork of lldb but the swift instructions do not mention anything special here, implying that it is added to the path.

@nrc
Copy link
Member

nrc commented Sep 4, 2018

Some thoughts:

Rust's lldb is optional. It is only available in nightly and it is not installed by default

We might in the long-run want to install it by default, ideally we wouldn't mess with the names then

Rust's lldb is actually just the standard lldb

User's might have non-standard lldbs installed though, e.g., Swift's.

3 4

These seem like good reasons.

We could tweak rust-lldb yeah or simply repurpose it on OSX to the actual binary (which should probably have the pretty printers anyway)?

This seems like a pretty good solution to me. @tromey do you think re-using rust-lldb would work? If not, I think we should just install as lldb - it is optional to install, and if that is the best way forward we could keep it optional.

@alexcrichton
Copy link
Member

@tromey ah yeah as @nrc mentioned I think we should plan for installing LLDB by default, and in that case it'd be in PATH by default and may override other LLDB installations. Also isn't ours "special" in that it doesn't do "the thing" with signing code on OSX, right? (does that mean it doesn't work for everything on OSX? or has that changed?)

I agree that we should try to avoid the weird failure cases, but I think we can do that by always saying that users should execute rust-lldb, right? That way if it's not installed they get a clear error explaining why.

I also sort of trust Swift to ship its own LLDB than us, they've got a lot more Apple people looking at it than we do :(

@tromey
Copy link
Contributor Author

tromey commented Sep 5, 2018

Also isn't ours "special" in that it doesn't do "the thing" with signing code on OSX, right?

The lldb build will pick up the xcode signed lldb-server rather than the unsigned one. Which is weird but has the side effect of making it work for us. As far as I can tell.

Any opinions on what would be a good name for our lldb? I am not sure I am ready yet to remove the rust-lldb script and rename "lldb" to "rust-lldb". Instead in the interim I think I would prefer to make rust-lldb adaptive, assuming that's possible somehow.

That way if it's not installed they get a clear error explaining why

They won't because stock lldb pretends that rust is c++ and will try to solider on. That is, with the current plan "lldb myprogram" will still do something, and a user may or may not notice until the debugger prints something incorrect.

@alexcrichton
Copy link
Member

If we're not ready to take oer rust-lldb, perhaps rust-llvm-preview? Alternatively we could just make this the "eventual plan" and for now bury it in the sysroot and use another script to execute it.

@tromey
Copy link
Contributor Author

tromey commented Sep 5, 2018

Ok, thanks. I'll come back around when the prerequisites are done.

@tromey tromey closed this Sep 5, 2018
@tromey
Copy link
Contributor Author

tromey commented Sep 5, 2018

If we don't make a wrapper at all, perhaps we can just keep the name lldb and have rust-lldb look for that name in the sysroot. This seems simplest of all.

@alexcrichton
Copy link
Member

Yeah that sounds reasonable to me!

@tromey
Copy link
Contributor Author

tromey commented Sep 5, 2018

rust-lang/rust#53973

@tromey tromey deleted the lldb branch September 6, 2018 17:51
tromey added a commit to tromey/rust that referenced this pull request Sep 7, 2018
We're shipping a rust-enabled lldb, but the "lldb" executable is not
installed into the "bin" directory by rustup.  See the discussion in
rust-lang/rustup#1492 for
background on this decision.  There, we agreed to have rust-lldb
prefer the rust-enabled lldb if it is installed.

This patch changes dist.rs to put lldb into rustlib, following what
was done for the other LLVM tools in rust-lang#53955, and then fixes rust-lldb
to prefer that lldb, if it exists.

See issue rust-lang#48168
kennytm added a commit to kennytm/rust that referenced this pull request Sep 7, 2018
…alexcrichton

Have rust-lldb look for the rust-enabled lldb

We're shipping a rust-enabled lldb, but the "lldb" executable is not
installed into the "bin" directory by rustup.  See the discussion in
rust-lang/rustup#1492 for
background on this decision.  There, we agreed to have rust-lldb
prefer the rust-enabled lldb if it is installed.  This patch changes
rust-lldb to look in the sysroot and use the lldb found there, if any.

See issue rust-lang#48168
kennytm added a commit to kennytm/rust that referenced this pull request Sep 8, 2018
…alexcrichton

Have rust-lldb look for the rust-enabled lldb

We're shipping a rust-enabled lldb, but the "lldb" executable is not
installed into the "bin" directory by rustup.  See the discussion in
rust-lang/rustup#1492 for
background on this decision.  There, we agreed to have rust-lldb
prefer the rust-enabled lldb if it is installed.  This patch changes
rust-lldb to look in the sysroot and use the lldb found there, if any.

See issue rust-lang#48168
kennytm added a commit to kennytm/rust that referenced this pull request Sep 8, 2018
…alexcrichton

Have rust-lldb look for the rust-enabled lldb

We're shipping a rust-enabled lldb, but the "lldb" executable is not
installed into the "bin" directory by rustup.  See the discussion in
rust-lang/rustup#1492 for
background on this decision.  There, we agreed to have rust-lldb
prefer the rust-enabled lldb if it is installed.  This patch changes
rust-lldb to look in the sysroot and use the lldb found there, if any.

See issue rust-lang#48168
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants