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

rustc_codegen_ssa: Don't drop IncorrectCguReuseType , make rustc_expected_cgu_reuse attr work #118973

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

Enselic
Copy link
Member

@Enselic Enselic commented Dec 15, 2023

In 100753, IncorrectCguReuseType accidentally stopped being emitted by removing diag.span_err(...). Begin emitting it again rather than just blindly dropping it, and adjust tests accordingly.

We assume that there are no bugs and that the currently actual CGU reuse is correct. If there are bugs, they will be discovered and fixed eventually, and the tests will then be updated.

Closes #118972

@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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. labels Dec 15, 2023
@klensy
Copy link
Contributor

klensy commented Dec 15, 2023

Spraying #[must_use] all over error structs should reveal similar places? Or adding it somewhere in Diagnostic derive.

@michaelwoerister
Copy link
Member

Great find, @Enselic! I'll take a look at this.
r? @michaelwoerister

@michaelwoerister
Copy link
Member

and adjust tests accordingly.

Hm, so this does seem to have regressed. Quite a few CGUs go from post-lto (which means we re-use the bitcode/object file after ThinLTO has been applied) to pre-lto (which means we have to redo the ThinLTO step).

@michaelwoerister
Copy link
Member

Looks like not emitting the error was introduced in 706452e (merged Aug 31, 2022).

I wonder when the test cases regressed though.

@michaelwoerister
Copy link
Member

tests/incremental/thinlto/independent_cgus_dont_affect_each_other.rs
tests/incremental/thinlto/cgu_keeps_identical_fn.rs
tests/incremental/thinlto/cgu_invalidated_via_import.rs

These seem to start failing after #115964 (which refactors CGU reuse tracking). @bjorn3, any ideas where the change in behavior could come from?

This one already fails before #115964:

tests/incremental/thinlto/cgu_keeps_identical_fn.rs

However, there the change is from no to post-lto, which is actually an improvement. I haven't verified yet that that's valid though.

@bjorn3
Copy link
Member

bjorn3 commented Dec 15, 2023

These seem to start failing after #115964 (which refactors CGU reuse tracking). @bjorn3, any ideas where the change in behavior could come from?

See the PR description:

It will now miss post-lto cgu reuse when ThinLTO determines that a cgu doesn't get changed, but there weren't any tests for this anyway and a test for it would be fragile to the exact implementation of ThinLTO in LLVM.

It seems there were tests after all, but because of this bug didn't actually fail. It is still fragile to the exact implementation of ThinLTO though. The tests would also almost certainly fail for cg_gcc and definitively fail for cg_clif.

@michaelwoerister
Copy link
Member

Does that mean that post-LTO artifacts can never be re-used?

@bjorn3
Copy link
Member

bjorn3 commented Dec 15, 2023

cg_clif doesn't do any LTO, so post-lto reusing never happens. As for cg_gcc, GCC doesn't have ThinLTO, instead it has something closer to fat LTO except that it can split it into multiple independent modules. The exact heuristics for this are different from LLVM's ThinLTO and thus would likely cause the tests to fail. Also cg_gcc doesn't have anything like thin-local-lto yet, which these tests rely on.

@michaelwoerister
Copy link
Member

Sorry for the confusion. What I meant was: Does #115964 unconditionally turn off post-LTO artifact caching when using LLVM with ThinLTO?

@michaelwoerister
Copy link
Member

From what I can tell, #115964 only changes whether postLTO reuse is reported to the tracker, but does not change the reuse behavior itself. I wonder if we can restore the reporting. You're right that this depends on the implementation of ThinLTO in LLVM, but I think there's value in having at least some testing here, right? I.e. there are some simple cases that we expect to keep working.

I'll open an issue about this. The testing situation could use some improvement here, also with respect to other backends. As it is right now, the tests are misleading because they don't reflect what's actually happening.

@bjorn3
Copy link
Member

bjorn3 commented Dec 15, 2023

Sorry for the confusion. What I meant was: Does #115964 unconditionally turn off post-LTO artifact caching when using LLVM with ThinLTO?

No it does not. It merely hides post-lto caching from the cgu_reuse_tracker. Previously whenever a post-lto artifact was reused, this fact would be written into the Session and at the end the cgu_reuse_tracker would read it all to determine which errors to emit. Now the cgu_reuse_tracker emits all errors right right after asking the incr comp system which codegen units need to be codegened again and before the actual codegen happens. Determining which post-lto artifacts are reused can only happen after all codegen units have their pre-lto module codegened/loaded from the incr cache.

I'll open an issue about this.

👍

@michaelwoerister
Copy link
Member

Thanks for taking a look, @bjorn3!

@Enselic, do you want to follow up on @klensy's comment? It seems like a great idea, but I could imagine that it leads to a number of fixes being necessary and should maybe done in a separate PR?

Otherwise this PR is good to go.

@Enselic
Copy link
Member Author

Enselic commented Dec 15, 2023

@michaelwoerister I now have looked a bit into changing the Diagnostic derive macro to add #[must_use] to the struct it derives for, but I don't think derive macros can change the struct itself, like adding an attribute to it? A derive macro can only add items (such as impls) after the struct, I think? Or am I confused?

I agree it is a good idea and that it would be nice to get it done.

@Enselic
Copy link
Member Author

Enselic commented Dec 15, 2023

Adding #[must_use] manually to each struct currently does not compile:

error: `#[must_use]` is not a valid attribute
  --> compiler/rustc_codegen_ssa/src/errors.rs:22:1
   |
22 | #[must_use]

Hmm maybe we could change Diagnostic so that it checks that #[must_use] has been manually added (after allowing it) so it is impossible to forget it...

@klensy
Copy link
Contributor

klensy commented Dec 15, 2023

@klensy
Copy link
Contributor

klensy commented Dec 15, 2023

error: unused `IncorrectCguReuseType` that must be used
   --> compiler\rustc_codegen_ssa\src\assert_module_sources.rs:281:25
    |
281 | /                         errors::IncorrectCguReuseType {
282 | |                             span: *error_span,
283 | |                             cgu_user_name,
284 | |                             actual_reuse,
285 | |                             expected_reuse,
286 | |                             at_least,
287 | |                         };
    | |_________________________^
    |
    = note: `-D unused-must-use` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(unused_must_use)]`
help: use `let _ = ...` to ignore the resulting value
    |
281 |                         let _ = errors::IncorrectCguReuseType {
    |                         +++++++

@michaelwoerister
Copy link
Member

@michaelwoerister I now have looked a bit into changing the Diagnostic derive macro to add #[must_use] to the struct it derives for, but I don't think derive macros can change the struct itself, like adding an attribute to it? A derive macro can only add items (such as impls) after the struct, I think? Or am I confused?

Indeed, a derive macro cannot change the type it is attached to. An attribute macro could do that. But changing the diag macro is something that wg-diagnostics would need to decide, I'd say.

Adding #[must_use] manually to each struct currently does not compile:

Interesting. Have you tried adding the attribute at different positions? That is:

#[must_use] 
#[derive(...)]
#[diag]
struct Foo {}

vs

#[derive(...)]
#[diag]
#[must_use] 
struct Foo {}

for example?

But I actually think that making the #[diag] macro add #[must_use] is the best course of action.

@Enselic Enselic force-pushed the fix-IncorrectCguReuseType branch from c25589a to 88b6ff7 Compare December 15, 2023 21:58
@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic labels Dec 15, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2023

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@Enselic
Copy link
Member Author

Enselic commented Dec 15, 2023

But I actually think that making the #[diag] macro add #[must_use] is the best course of action.

The #[diag] macro is not an attribute macro but a Diagnostic derive macro helper attribute, so I don't think it can add #[must_use]?

I pushed commits now to make #[must_use] mandatory for struct diagnostics (I skipped enum diagnostics and Subdiagnostics for now). See it as a prototype that can be merged if it looks OK.

You'll want to review individual commits since the full diff is >1000 lines now (which is annoying and I would love to not have to do that if there is a way to avoid it).

(I can't do it in a separate PR yet since it won't build without the fix in this PR.)

@rust-log-analyzer

This comment has been minimized.

@Enselic Enselic force-pushed the fix-IncorrectCguReuseType branch from 88b6ff7 to 2f1a031 Compare December 15, 2023 22:13
@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

@Enselic Enselic force-pushed the fix-IncorrectCguReuseType branch from 2f1a031 to 3c7f6b1 Compare December 16, 2023 05:30
@rust-log-analyzer

This comment has been minimized.

@Enselic Enselic force-pushed the fix-IncorrectCguReuseType branch from 3c7f6b1 to 2ad44f8 Compare December 16, 2023 08:19
@klensy
Copy link
Contributor

klensy commented Dec 16, 2023

Enums with Diagnostic too?

@Enselic
Copy link
Member Author

Enselic commented Dec 16, 2023

This PR makes

  • struct Diagnostic

require must_use. A natural follow-up is to also enforce this on

  • enum Diagnostic
  • struct Subdiagnostic
  • enum Subdiagnostic

but let's wait with that until we know we want this at all. I'd rather not spend time on implementing that only to then have to throw it all in the bin.

@bors
Copy link
Contributor

bors commented Dec 18, 2023

☔ The latest upstream changes (presumably #119063) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelwoerister
Copy link
Member

I've opened #119076 to document that CGU reuse tracking only reports pre-LTO reuse.

This PR makes struct Diagnostic require must_use.

I actually think that it would be better to

  • make a proc-macro add the must_use attribute, which will likely involve switching to an attribute macro, and should definitely be coordinated with @rust-lang/wg-diagnostics beforehand. Or add a lint that catches this. In any case, we'll not want to rely on folks remembering to add must_use manually for each new instance of a diagnostic.
  • focus this PR on fixing this particular issue in CGU reuse tracking, so that those test go online again asap.

What do you think, @Enselic?

@Enselic Enselic force-pushed the fix-IncorrectCguReuseType branch from 2ad44f8 to 43491ec Compare December 18, 2023 20:33
@Enselic
Copy link
Member Author

Enselic commented Dec 18, 2023

In any case, we'll not want to rely on folks remembering to add must_use manually for each new instance of a diagnostic.

To be clear, with the code I had it is impossible to forget must_use, because if you forget must_use on a struct you get this error at compile time:

error: You must mark diagnostic structs with `#[must_use]`
  --> compiler/rustc_ast_lowering/src/errors.rs:69:1
   |
69 | #[diag(ast_lowering_assoc_ty_parentheses)]
   | ^

but I completely agree we should get the original bugfix of this PR merged before we continue to iterate on the must_use-solution. I forced pushed away the must_use-commits now.

@michaelwoerister
Copy link
Member

To be clear, with the code I had it is impossible to forget must_use, because if you forget must_use on a struct you get this error at compile time

Oh, you made the #[Diagnostic] macro emit an error if must_use isn't present? That seems like a good, lightweight solution!

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested comments linking to the issue.

Otherwise, this looks ready to merge, r=me. Thanks again for uncovering this, @Enselic!

In [100753], `IncorrectCguReuseType` accidentally stopped being emitted.
Begin emitting it again rather than just blindly dropping it, and adjust
tests accordingly.

[100753]: rust-lang@706452e#diff-048389738ddcbe0f9765291a29db1fed9a5f03693d4781cfb5aaa97ffb3c7f84
@Enselic Enselic force-pushed the fix-IncorrectCguReuseType branch from 38d40c2 to d46df80 Compare December 19, 2023 19:21
@michaelwoerister
Copy link
Member

Thanks, @Enselic!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 19, 2023

📌 Commit d46df80 has been approved by michaelwoerister

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 Dec 19, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Dec 19, 2023
… r=michaelwoerister

rustc_codegen_ssa: Don't drop `IncorrectCguReuseType` , make `rustc_expected_cgu_reuse` attr work

In [100753], `IncorrectCguReuseType` accidentally stopped being emitted by removing `diag.span_err(...)`. Begin emitting it again rather than just blindly dropping it, and adjust tests accordingly.

We assume that there are no bugs and that the currently actual CGU reuse is correct. If there are bugs, they will be discovered and fixed eventually, and the tests will then be updated.

[100753]: rust-lang@706452e#diff-048389738ddcbe0f9765291a29db1fed9a5f03693d4781cfb5aaa97ffb3c7f84

Closes rust-lang#118972
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 19, 2023
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#118691 (Add check for possible CStr literals in pre-2021)
 - rust-lang#118973 (rustc_codegen_ssa: Don't drop `IncorrectCguReuseType` , make `rustc_expected_cgu_reuse` attr work)
 - rust-lang#119071 (-Znext-solver: adapt overflow rules to avoid breakage)
 - rust-lang#119089 (effects: fix a comment)
 - rust-lang#119096 (Yeet unnecessary param envs)
 - rust-lang#119118 (Fix arm64e-apple-ios target)
 - rust-lang#119134 (resolve: Feed visibilities for unresolved trait impl items)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 20, 2023
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#118691 (Add check for possible CStr literals in pre-2021)
 - rust-lang#118973 (rustc_codegen_ssa: Don't drop `IncorrectCguReuseType` , make `rustc_expected_cgu_reuse` attr work)
 - rust-lang#119071 (-Znext-solver: adapt overflow rules to avoid breakage)
 - rust-lang#119089 (effects: fix a comment)
 - rust-lang#119096 (Yeet unnecessary param envs)
 - rust-lang#119118 (Fix arm64e-apple-ios target)
 - rust-lang#119134 (resolve: Feed visibilities for unresolved trait impl items)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 20, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#118691 (Add check for possible CStr literals in pre-2021)
 - rust-lang#118973 (rustc_codegen_ssa: Don't drop `IncorrectCguReuseType` , make `rustc_expected_cgu_reuse` attr work)
 - rust-lang#119071 (-Znext-solver: adapt overflow rules to avoid breakage)
 - rust-lang#119089 (effects: fix a comment)
 - rust-lang#119094 (Add function ABI and type layout to StableMIR)
 - rust-lang#119102 (Add arm-none-eabi and armv7r-none-eabi platform-support documentation.)
 - rust-lang#119107 (subtype_predicate: remove unnecessary probe)

Failed merges:

 - rust-lang#119135 (Fix crash due to `CrateItem::kind()` not handling constructors)
 - rust-lang#119141 (Add method to get instance instantiation arguments)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit df4d563 into rust-lang:master Dec 20, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 20, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 20, 2023
Rollup merge of rust-lang#118973 - Enselic:fix-IncorrectCguReuseType, r=michaelwoerister

rustc_codegen_ssa: Don't drop `IncorrectCguReuseType` , make `rustc_expected_cgu_reuse` attr work

In [100753], `IncorrectCguReuseType` accidentally stopped being emitted by removing `diag.span_err(...)`. Begin emitting it again rather than just blindly dropping it, and adjust tests accordingly.

We assume that there are no bugs and that the currently actual CGU reuse is correct. If there are bugs, they will be discovered and fixed eventually, and the tests will then be updated.

[100753]: rust-lang@706452e#diff-048389738ddcbe0f9765291a29db1fed9a5f03693d4781cfb5aaa97ffb3c7f84

Closes rust-lang#118972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incremental ThinLTO tests attribute rustc_expected_cgu_reuse ignored
8 participants