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

Bootstrap: Don't get output if lldb --version errors #127112

Merged
merged 2 commits into from
Jun 29, 2024

Conversation

ChrisDenton
Copy link
Member

fixes #126892

Command can error in two ways: the OS can fail to run the binary at all or else the binary can return an error exit code. Unfortunately the distinction between the two is not clear cut. The OS may succeed in starting the binary but it may still error before main (e.g. if a necessary library fails to load) and this will be reported via the exit code.

Fortunately this case is simpler. We can assume that lldb --version will only ever error if there's a startup issue of some kind. so both kinds of errors are caused by the OS. Thus it's safe for us to treat them equally for the sake of this specific check.

@rustbot
Copy link
Collaborator

rustbot commented Jun 29, 2024

r? @Kobzol

rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 29, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Jun 29, 2024

I don't understand how this fixes the crash. It seems just to change the returned error? The original issue showed a crash on line 1820, inside the run function, but that isn't used in this lldb invocation. It is probably crashing elsewhere (?) :) Probably line 1841.

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Jun 29, 2024

Sorry, I didn't explain that very well. The code is a bit spaghetti but the run function is only invoked after the lldb --version is, It's guarded by the if let Some(ref vers) = lldb_version { block.

And lldb --version already fails but we don't actually check the exit status before reading the output of lldb --version so we're getting nonsense. Well probably Some("") because it doesn't print anything.

@ChrisDenton
Copy link
Member Author

I've pushed a new commit that hopefully makes the code a bit clearer.

@Kobzol
Copy link
Contributor

Kobzol commented Jun 29, 2024

Aah, I understand now, thanks! Yeah, this command invocation code is very messy. I will nuke it soon (probably in the next couple of days) anyway with my bootstrap command refactoring PRs. But it doesn't hurt to fix the issue for now, it will help me understand the intent of the original code better.

Thanks!

@bors rollup r+

@bors
Copy link
Contributor

bors commented Jun 29, 2024

📌 Commit a6ef91e has been approved by Kobzol

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 29, 2024
@ChrisDenton
Copy link
Member Author

I will nuke it soon (probably in the next couple of days) anyway with my bootstrap command refactoring PRs.

That'll be great!

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 29, 2024
…llaumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#126805 (Migrate `pdb-alt-path`, `mismatching-target-triples` and `mingw-export-call-convention` `run-make` tests to rmake)
 - rust-lang#126995 (Migrate `pretty-print-with-dep-file`, `pretty-print-to-file` and `libtest-padding` `run-make` tests to rmake)
 - rust-lang#127041 (Migrate `run-make/override-aliased-flags` to `rmake.rs`)
 - rust-lang#127072 (docs: say "includes" instead of "does include")
 - rust-lang#127073 (Remove unnecessary SeqCst in `impl fmt::Pointer for AtomicPtr`)
 - rust-lang#127112 (Bootstrap: Don't get output if `lldb --version` errors)
 - rust-lang#127116 (Migrate `run-make/return-non-c-like-enum` to `rmake.rs`)

Failed merges:

 - rust-lang#127050 (Make mtime of reproducible tarballs dependent on git commit)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 38983df into rust-lang:master Jun 29, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 29, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 29, 2024
Rollup merge of rust-lang#127112 - ChrisDenton:lldb, r=Kobzol

Bootstrap: Don't get output if `lldb --version` errors

fixes rust-lang#126892

`Command` can error in two ways: the OS can fail to run the binary at all or else the binary can return an error exit code. Unfortunately the distinction between the two is not clear cut. The OS may succeed in starting the binary but it may still error before `main` (e.g. if a necessary library fails to load) and this will be reported via the exit code.

Fortunately this case is simpler. We can assume that `lldb --version` will only ever error if there's a startup issue of some kind. so both kinds of errors are caused by the OS. Thus it's safe for us to treat them equally for the sake of this specific check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bootstrap panics if lldb fails to start
4 participants