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

Rollup of 10 pull requests #91760

Merged
merged 23 commits into from
Dec 11, 2021
Merged

Rollup of 10 pull requests #91760

merged 23 commits into from
Dec 11, 2021

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

jhpratt and others added 23 commits November 14, 2021 00:53
As discovered in rust-lang#85461, the MSVC linker treats weak symbols slightly
differently than unix-y linkers do. This causes link.exe to fail with
LNK1227 "conflicting weak extern definition" where as other targets are
able to link successfully.

This changes the dead functions from being generated as weak/hidden to
private/default which, as the LLVM reference says:

> Global values with “private” linkage are only directly accessible by
objects in the current module. In particular, linking code into a module
with a private global value may cause the private to be renamed as
necessary to avoid collisions. Because the symbol is private to the
module, all references can be updated. This doesn’t show up in any
symbol table in the object file.

This fixes the conflicting weak symbols but doesn't address the reason
*why* we have conflicting symbols for these dead functions. The test
cases added in this commit contain a minimal repro of the fundamental
issue which is that the logic used to decide what dead code functions
should be codegen'd in the current CGU doesn't take into account that
functions can be duplicated across multiple CGUs (for instance, in the
case of `#[inline(always)]` functions).

Fixing that is likely to be a more complex change (see
rust-lang#85461 (comment)).

Fixes rust-lang#85461
Also:

- Review and edit current docs
- Enforce documentation for crate

Co-authored-by: r00ster <[email protected]>
Co-authored-by: Camille Gillot <[email protected]>
…s, r=cjgillot

Document all public items in `rustc_incremental`

Also:

- Review and edit current docs
- Enforce documentation for the module.
…, r=dtolnay

Fix incorrect stability attributes

These two instances were caught in rust-lang#90356, but that PR isn't going to be merged. I've extracted these to ensure it's still correct.

``@rustbot`` label: +A-stability +C-cleanup +S-waiting-on-review
Fix method name reference in stream documentation
adjust const_eval_select documentation

"The Rust compiler assumes" indicates that this is language UB, but [I don't think that is a good idea](https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/const_eval_select.20assumptions). This UB would be very hard to test for and looks like a way-too-big footgun. ``@oli-obk`` suggested this is meant to be more like "library UB", so I tried to adjust the docs accordingly.

I also removed all references to "referential transparency". That is a rather vague concept used to mean many different things, and I honestly have no idea what exactly is meant by it in this specific instance. But I assume ``@fee1-dead`` had in their mind a property that all `const fn` code upholds, so by demanding that the runtime code and the const-time code are *observably equivalent*, whatever that property is would also be enforced here.

Cc ``@rust-lang/wg-const-eval``
…r, r=tmandry

code-cov: generate dead functions with private/default linkage

As discovered in rust-lang#85461, the MSVC linker treats weak symbols slightly
differently than unix-y linkers do. This causes link.exe to fail with
LNK1227 "conflicting weak extern definition" where as other targets are
able to link successfully.

This changes the dead functions from being generated as weak/hidden to
private/default which, as the LLVM reference says:

> Global values with “private” linkage are only directly accessible by
objects in the current module. In particular, linking code into a module
with a private global value may cause the private to be renamed as
necessary to avoid collisions. Because the symbol is private to the
module, all references can be updated. This doesn’t show up in any
symbol table in the object file.

This fixes the conflicting weak symbols but doesn't address the reason
*why* we have conflicting symbols for these dead functions. The test
cases added in this commit contain a minimal repro of the fundamental
issue which is that the logic used to decide what dead code functions
should be codegen'd in the current CGU doesn't take into account that
functions can be duplicated across multiple CGUs (for instance, in the
case of `#[inline(always)]` functions).

Fixing that is likely to be a more complex change (see
rust-lang#85461 (comment)).

Fixes rust-lang#85461
…eeMap-documentation, r=yaahc

Update documentation to use `from()` to initialize `HashMap`s and `BTreeMap`s

As of Rust 1.56, `HashMap` and `BTreeMap` both have associated `from()` functions.  I think using these in the documentation cleans things up a bit.  It allows us to remove some of the `mut`s and avoids the Initialize-Then-Modify anti-pattern.
…=dtolnay

Fix Vec::extend_from_slice docs

`other` is a slice not a vector.
…llot

Fix ICE on format string of macro with secondary-label

This generalizes the fix rust-lang#86104 to also correctly skip `Span::from_inner` for the `secondary_label` of a format macro parsing error as well.

We can alternatively skip the `span_label` diagnostic call for the secondary label as well, since that label probably only makes sense when the _proper_ span is computed.

Fixes rust-lang#91556
Fix documentation for `core::ready::Ready`
@rustbot rustbot added the rollup A PR which is a rollup label Dec 10, 2021
@matthiaskrgr
Copy link
Member Author

@bors r+ p=11

@bors
Copy link
Contributor

bors commented Dec 10, 2021

📌 Commit 1fca934 has been approved by matthiaskrgr

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 10, 2021
@bors
Copy link
Contributor

bors commented Dec 10, 2021

⌛ Testing commit 1fca934 with merge f0448f4...

@bors
Copy link
Contributor

bors commented Dec 11, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 11, 2021
@bors bors merged commit f0448f4 into rust-lang:master Dec 11, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 11, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f0448f4): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

  • Very large regression in instruction counts (up to 5.5% on incr-unchanged builds of inflate)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

@rustbot rustbot added the perf-regression Performance regression. label Dec 11, 2021
@matthiaskrgr
Copy link
Member Author

Hmm, there is nothing that jumps at me at me right away.
It's strange that there are no changes in other benchmarks but the change seems a bit too big to be noise :D
Expanding took a bit longer, #91575 changes expansion so maybe this one is the culprit?

@matthiaskrgr
Copy link
Member Author

Looks like the regression went away again in #91715 (comment)
so probably spurious 🤷

@matthiaskrgr matthiaskrgr deleted the rollup-zcemh6j branch December 12, 2021 09:58
@pnkfelix
Copy link
Member

I think this is indeed spurious. I'm removing both #91760 and #91715 from this week's performance triage report.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.