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

Target modifiers (special marked options) are recorded in metainfo #133138

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

azhogin
Copy link
Contributor

@azhogin azhogin commented Nov 17, 2024

Target modifiers (special marked options) are recorded in metainfo and compared to be equal in different linked crates.

PR for this RFC: rust-lang/rfcs#3716

Option may be marked as TARGET_MODIFIER, example: regparm: Option<u32> = (None, parse_opt_number, [TRACKED TARGET_MODIFIER].
If an TARGET_MODIFIER-marked option has non-default value, it will be recorded in crate metainfo as a Vec<TargetModifier>:

pub struct TargetModifier {
    pub opt: OptionsTargetModifiers,
    pub value_name: String,
}

OptionsTargetModifiers is a macro-generated enum.

Option value code (for comparison) is generated using Debug trait.

Error example:

error: mixing `-Zregparm` will cause an ABI mismatch in crate `incompatible_regparm`
  --> $DIR/incompatible_regparm.rs:10:1
   |
LL | #![crate_type = "lib"]
   | ^
   |
   = help: the `-Zregparm` flag modifies the ABI so Rust crates compiled with different values of this flag cannot be used together safely
   = note: `-Zregparm=1` in this crate is incompatible with `-Zregparm=2` in dependency `wrong_regparm`
   = help: set `-Zregparm=2` in this crate or `-Zregparm=1` in `wrong_regparm`
   = help: if you are sure this will not cause problems, use `-Cunsafe-allow-abi-mismatch=regparm` to silence this error

error: aborting due to 1 previous error

-Cunsafe-allow-abi-mismatch=regparm,reg-struct-return to disable list of flags.

@rustbot
Copy link
Collaborator

rustbot commented Nov 17, 2024

r? @davidtwco

rustbot has assigned @davidtwco.
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 17, 2024
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/target-modifiers branch from d99ff62 to bd52a23 Compare November 18, 2024 19:21
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/target-modifiers branch from bd52a23 to 500600b Compare November 18, 2024 22:37
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/target-modifiers branch 2 times, most recently from db91299 to 43e5956 Compare November 20, 2024 22:32
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/target-modifiers branch from 43e5956 to 6793451 Compare November 21, 2024 10:31
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2024
@azhogin azhogin force-pushed the azhogin/target-modifiers branch 2 times, most recently from 95f9595 to 9cd86c3 Compare November 26, 2024 22:28
@azhogin
Copy link
Contributor Author

azhogin commented Nov 28, 2024

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 28, 2024
@azhogin azhogin marked this pull request as ready for review November 28, 2024 10:54
@azhogin azhogin force-pushed the azhogin/target-modifiers branch from 9cd86c3 to 97a8240 Compare November 28, 2024 11:57
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/target-modifiers branch from 97a8240 to 8603b2b Compare November 28, 2024 13:12
@bors

This comment was marked as resolved.

@azhogin azhogin force-pushed the azhogin/target-modifiers branch from e3aa87a to 05c88a3 Compare February 2, 2025 15:12
@saethlin
Copy link
Member

saethlin commented Feb 2, 2025

@bors r=davidtwco,saethlin

@bors
Copy link
Contributor

bors commented Feb 2, 2025

📌 Commit 05c88a3 has been approved by davidtwco,saethlin

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 Feb 2, 2025
@bors
Copy link
Contributor

bors commented Feb 3, 2025

⌛ Testing commit 05c88a3 with merge 73e2412...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 3, 2025
…davidtwco,saethlin

Target modifiers (special marked options) are recorded in metainfo

Target modifiers (special marked options) are recorded in metainfo and compared to be equal in different linked crates.

PR for this RFC: rust-lang/rfcs#3716

Option may be marked as `TARGET_MODIFIER`, example: `regparm: Option<u32> = (None, parse_opt_number, [TRACKED TARGET_MODIFIER]`.
If an TARGET_MODIFIER-marked option has non-default value, it will be recorded in crate metainfo as a `Vec<TargetModifier>`:
```
pub struct TargetModifier {
    pub opt: OptionsTargetModifiers,
    pub value_name: String,
}
```

OptionsTargetModifiers is a macro-generated enum.

Option value code (for comparison) is generated using `Debug` trait.

Error example:
```
error: mixing `-Zregparm` will cause an ABI mismatch in crate `incompatible_regparm`
  --> $DIR/incompatible_regparm.rs:10:1
   |
LL | #![crate_type = "lib"]
   | ^
   |
   = help: the `-Zregparm` flag modifies the ABI so Rust crates compiled with different values of this flag cannot be used together safely
   = note: `-Zregparm=1` in this crate is incompatible with `-Zregparm=2` in dependency `wrong_regparm`
   = help: set `-Zregparm=2` in this crate or `-Zregparm=1` in `wrong_regparm`
   = help: if you are sure this will not cause problems, use `-Cunsafe-allow-abi-mismatch=regparm` to silence this error

error: aborting due to 1 previous error
```

`-Cunsafe-allow-abi-mismatch=regparm,reg-struct-return` to disable list of flags.
@rust-log-analyzer

This comment was marked as resolved.

@bors

This comment was marked as resolved.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 3, 2025
@saethlin saethlin added the CI-spurious-fail-msvc CI spurious failure: target env msvc label Feb 3, 2025
@saethlin
Copy link
Member

saethlin commented Feb 3, 2025

@bors retry

@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 Feb 3, 2025
@bors
Copy link
Contributor

bors commented Feb 3, 2025

⌛ Testing commit 05c88a3 with merge 7daf4cf...

@bors
Copy link
Contributor

bors commented Feb 3, 2025

☀️ Test successful - checks-actions
Approved by: davidtwco,saethlin
Pushing 7daf4cf to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 3, 2025
@bors bors merged commit 7daf4cf into rust-lang:master Feb 3, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 3, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7daf4cf): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

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

Cycles

Results (primary 4.5%)

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)
4.5% [4.5%, 4.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 4.5% [4.5%, 4.5%] 1

Binary size

Results (secondary 0.0%)

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.0% [0.0%, 0.0%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Bootstrap: 778.369s -> 779.014s (0.08%)
Artifact size: 328.65 MiB -> 328.74 MiB (0.03%)

@petrochenkov
Copy link
Contributor

The tests are unignored on all platforms in #137801.

@petrochenkov
Copy link
Contributor

Also, the amount of over-engineering in the implementation here is very high.

There will only be several options like this, half of them will likely use some individual approach and smart comparison.
So there's no need to introduce the TARGET_MODIFIER syntax and parse it using elaborated tt munchers.
There's also no need to generalize over various modifiers to put them into a single map or vector.

A structure with a manually written field per every modifier should work well enough here.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 1, 2025
…rors

tests: Unignore target modifier tests on all platforms

These tests can be `check-pass` and do not need dynamic libraries.
Also remove other unnecessary stuff from them.

Follow up to rust-lang#133138.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 2, 2025
…rors

tests: Unignore target modifier tests on all platforms

These tests can be `check-pass` and do not need dynamic libraries.
Also remove other unnecessary stuff from them.

Follow up to rust-lang#133138.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 2, 2025
…rors

tests: Unignore target modifier tests on all platforms

These tests can be `check-pass` and do not need dynamic libraries.
Also remove other unnecessary stuff from them.

Follow up to rust-lang#133138.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 3, 2025
…rors

tests: Unignore target modifier tests on all platforms

These tests can be `check-pass` and do not need dynamic libraries.
Also remove other unnecessary stuff from them.

Follow up to rust-lang#133138.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2025
Rollup merge of rust-lang#137801 - petrochenkov:tarmod, r=compiler-errors

tests: Unignore target modifier tests on all platforms

These tests can be `check-pass` and do not need dynamic libraries.
Also remove other unnecessary stuff from them.

Follow up to rust-lang#133138.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-metadata Area: Crate metadata CI-spurious-fail-msvc CI spurious failure: target env msvc merged-by-bors This PR was explicitly merged by bors. 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.