Skip to content

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

lcnr and others added 15 commits July 10, 2023 15:17
This setting was added to match rustfmt, but it's been taking effect on
all file editing, which I notice most on git `COMMIT_EDITMSG`. I want to
keep my default 72-width commit messages, please. :)
because sometimes users can't opt out
avoid building proof trees in select

otherwise we ICE because select isn't currently set up to print proof trees.

r? `````@BoxyUwU`````
Only use max_line_length = 100 for *.rs

This setting was added to match rustfmt, but it's been taking effect on
all file editing, which I notice most on git `COMMIT_EDITMSG`. I want to
keep my default 72-width commit messages, please. :)
refactor proof tree formatting

mostly:
- handle indentation via a separate formatter
- change nested to use a closure

tested it after rebasing on top of rust-lang#113536 and everything looks good.

r? `````@BoxyUwU`````
…notriddle

Add jump to doc

I'm using the source code pages of the compiler quite a lot, but one thing missing is the possibility to jump back from the source code to the item documentation. Since I also got a few others complaining about it, I think it's fine to add it since this option is nightly only.

This PR adds a link to jump back to item's documentation on the item definition (so on `Bar` in `struct Bar {... }`, as described in the unofficial [RFC](https://github.com/GuillaumeGomez/rfcs/blob/rustdoc-jump-to-definition/text/000-rustdoc-jump-to-definition.md)).

r? ````@notriddle````
make MCP510 behavior opt-in to avoid conflicts between the CLI and target flavors

Fixes rust-lang#113597, which contains more details on how this happens through the code, and showcases an unexpected `Gnu(Cc::Yes, Lld::Yes)` flavor.

rust-lang#112910 added support to use `lld` when the flavor requests it, but didn't explicitly do so only when using `-Clink-self-contained=+linker` or one of the unstable `-Clinker-flavor`s.

The problem: some targets have a `lld` linker and flavor, e.g. `thumbv6m-none-eabi` from that issue. Users can override the linker but there are no linker flavors precise enough to describe the linker opting out of lld: when using `-Clinker=arm-none-eabi-gcc`, we infer this is a `Cc::Yes` linker flavor, but the `lld` component is unknown and therefore defaulted to the target's linker flavor, `Lld::Yes`.

<details>
<summary>Walkthrough of how this happens</summary>

The linker flavor used is a mix between what can be inferred from the CLI (`-C linker`) and the target's default linker flavor:

- there is no linker flavor on the CLI (and that also offers another workaround on nightly: `-C linker-flavor=gnu-cc -Zunstable-options`), so it will have to be inferred [from here](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_codegen_ssa/src/back/link.rs#L1334-L1336) to [here](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_codegen_ssa/src/back/link.rs#L1321-L1327).
- in [`infer_linker_hints`](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_target/src/spec/mod.rs#L320-L352) `-C linker=arm-none-eabi-gcc` infers a `Some(Cc::Yes)` cc hint, and no hint about lld.
- the target's `linker_flavor` is combined in `with_cli_hints` with these hints. We have our `Cc::Yes`, but there is no hint about lld, [so the target's flavor `lld` component is used](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_target/src/spec/mod.rs#L356-L358). It's [`Gnu(Cc::No, Lld::Yes)`](https://github.com/rust-lang/rust/blob/993deaa0bf8bab9dd3eadfd1fbeb093328e95afe/compiler/rustc_target/src/spec/thumb_base.rs#L35).
- so we now have our `Gnu(Cc::Yes, Lld::Yes)` flavor

</details>

This results in a `Gnu(Cc::Yes, Lld::Yes)` flavor on a non-lld linker, causing an additional unexpected `-fuse-ld=lld` argument to be passed.

I don't know if this target defaulting to `rust-lld` is expected, but until MCP510's new linker flavor are stable, when people will be able to describe their linker/flavor accurately, this PR keeps the stable behavior of not doing anything when the linker/flavor on the CLI unexpectedly conflict with the target's.

I've tested this on a `no_std` `-C linker=arm-none-eabi-gcc -C link-arg=-nostartfiles --target thumbv6m-none-eabi` example, trying to simulate one of `cortex-m`'s test mentioned in issue rust-lang#113597 (I don't know how to build a local complete  `thumbv6m-none-eabi` toolchain to run the exact test), and checked that `-fuse-lld` was indeed gone and the error disappeared.

r? `````@petrochenkov`````
@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) rollup A PR which is a rollup labels Jul 13, 2023
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=6

@bors
Copy link
Collaborator

bors commented Jul 13, 2023

📌 Commit fc1cb04 has been approved by matthiaskrgr

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 Jul 13, 2023
@bors
Copy link
Collaborator

bors commented Jul 13, 2023

⌛ Testing commit fc1cb04 with merge 7bd81ee...

@bors
Copy link
Collaborator

bors commented Jul 13, 2023

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing 7bd81ee to master...

1 similar comment
@bors
Copy link
Collaborator

bors commented Jul 13, 2023

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing 7bd81ee to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 13, 2023
@bors bors merged commit 7bd81ee into rust-lang:master Jul 13, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 13, 2023
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#113536 avoid building proof trees in select ace66860c89e320ef8bf6831ea326818d86da5c4 (link)
#113558 Only use max_line_length = 100 for *.rs f6187869cc0b24727c17f6808b7e743201342cd3 (link)
#113570 refactor proof tree formatting 500a59722864e90248e9b22358aadb0ea20597da (link)
#113623 Add jump to doc ef7ad853d355f408a8b1ee8a007bd8c4be4f9838 (link)
#113629 Add Adt to SMIR 1d67978737c8bb8e751a6a1e7186ab5e0d971f5a (link)
#113631 make MCP510 behavior opt-in to avoid conflicts between the … 943a615e93bd1ff89fd63079517bdd7ce6a20023 (link)

previous master: a161ab00db

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7bd81ee): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.2% [1.2%, 1.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.2% [1.2%, 1.2%] 1

Max RSS (memory usage)

Results

This 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.8% [-2.9%, -2.8%] 2
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 658.555s -> 658.412s (-0.02%)

@rustbot rustbot added the perf-regression Performance regression. label Jul 14, 2023
@lqd
Copy link
Member

lqd commented Jul 14, 2023

This is a doc regression on html5ever, so possibly #113623. That benchmark looks usually stable and noise is less likely.

However, benchmarks weren't supposed to be affected by that PR if I understand correctly; maybe some of the additional work happens on regular codepaths, and not only when source links are on.

This is probably fine but let's cc PR author @GuillaumeGomez and reviewer @notriddle just in case.

@GuillaumeGomez
Copy link
Member

The change in #113623 is behind a disabled-by-default option, so it seems very unlikely to come from this one.

@lqd
Copy link
Member

lqd commented Jul 14, 2023

It's the only PR that looks like it could impact a doc benchmark in this rollup.

@GuillaumeGomez
Copy link
Member

Then no clue.

@lqd
Copy link
Member

lqd commented Jul 14, 2023

Let's launch a perf run for the PR just in case it's noise.

@rust-timer build ef7ad853d355f408a8b1ee8a007bd8c4be4f9838


Is the perf hit acceptable to T-rustdoc ? Or would the team like to investigate whether some codepaths were changed that were not protected by some verification that the option in question is enabled ? If the regression is acceptable/expected/not worth investigating, simply mark this PR as perf-regression-triaged.

@rust-timer

This comment has been minimized.

@GuillaumeGomez GuillaumeGomez added the perf-regression-triaged The performance regression has been triaged. label Jul 14, 2023
@GuillaumeGomez
Copy link
Member

We have a suspicion about an unnecessary string allocation. Sending a fix.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ef7ad853d355f408a8b1ee8a007bd8c4be4f9838): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.2% [1.2%, 1.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.2% [1.2%, 1.2%] 1

Max RSS (memory usage)

Results

This 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.0% [-3.3%, -2.8%] 4
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 658.555s -> 658s (-0.08%)

@matthiaskrgr matthiaskrgr deleted the rollup-zcume6k branch March 16, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants