Uh oh!
There was an error while loading. Please reload this page.
WIP: bootstrap: remove use-lld - #116182
Conversation
Removes the `use-lld` flag, which doesn't seem very useful.
rustbot
commented
Sep 26, 2023
(rustbot has picked a reviewer for you, use r? to override) |
Kobzol
commented
Sep 26, 2023
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
bors
commented
Sep 26, 2023
…try> WIP: bootstrap: remove use-lld Removes the `use-lld` flag, which doesn't seem very useful - it's only used on one place in our CI explicitly, and setting of `lld` is far from being as simple as a boolean option. I think that we should just use lld based on the target specs or other inferred LLD usage. Opening a PR to see what CI has to say about it.
bors
commented
Sep 26, 2023
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
rust-timer
commented
Sep 27, 2023
Finished benchmarking commit (6d68f23): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 633.132s -> 634.219s (0.17%) |
petrochenkov
commented
Sep 27, 2023
Why? |
Kobzol
commented
Sep 27, 2023
Ok! I'd like to know how it's being used. On which targets do you use In terms of our CI, we only use it for x64 Linux, and that target should hopefully switch soon to using LLD by default, so then the only usage of the flag would be removed. Regarding the flag itself, I think that it's a bit messy at the moment. At the same time, it enables the usage of LLD linker flags, and also tells bootstrap to use To clarify, I want to keep some way of telling bootstrap to use I think that one way could be to introduce something like this in bootstrap: enumLldConfig{Disabled,// don't use lldExternal(PathBuf),// use externally provided lldLocallyBuilt,// use lld built from src/llvm-projectSnapshot// use lld from the snapshot compiler}or, alternatively, just set |
mati865
commented
Sep 27, 2023
I suppose it's about Windows targets. |
ChrisDenton
commented
Sep 27, 2023
For msvc, lld should be a drop in replacement but there have been issues in practice. It's not something we test so arguably it's not technically supported. I believe Chromium use it tho and do report issues. |
petrochenkov
commented
Sep 28, 2023
On windows-gnu LLD may not work good enough to cover all corner cases, but it certainly works good enough to build and test rustc.
I also have such a hope, but I wouldn't put my money on it. |
petrochenkov
commented
Sep 28, 2023
I'm not against refactoring the options, but I would like to keep the existing functionality. |
Kobzol
commented
Sep 28, 2023
I think that If you use the global |
petrochenkov
commented
Sep 28, 2023
For me it doesn't matter match whether snapshot lld or global lld is used, snapshot lld would even be more convenient - no need to install anything globally. |
Kobzol
commented
Sep 28, 2023
Yeah, the reason why it doesn't work is #102101 and specifically this line. #116197 should fix that. |
petrochenkov
commented
Sep 28, 2023
No, it was (long?) before that. |
Kobzol
commented
Sep 28, 2023
Aha, I see. I guess that something like this might also be problematic. |
petrochenkov
commented
Sep 28, 2023
Okay, it didn't take much time, here's the first failure. Log |
Kobzol
commented
Sep 28, 2023
Ok, so it can't find |
petrochenkov
commented
Sep 28, 2023
You can look how |
Kobzol
commented
Sep 28, 2023
Thanks, I'll take a look at |
Removes the
use-lldflag, which doesn't seem very useful - it's only used on one place in our CI explicitly, and setting oflldis far from being as simple as a boolean option. I think that we should just use lld based on the target specs or other inferred LLD usage. Opening a PR to see what CI has to say about it.