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

Rearrange default_configuration and CheckCfg::fill_well_known. #118494

Merged

Conversation

nnethercote
Copy link
Contributor

There are comments saying these two functions should be kept in sync, but they have very different structures, process symbols in different orders, and there are some inconsistencies.

This commit reorders them so they're both mostly processing symbols in alphabetical order, which makes cross-checking them a lot easier. The commit also adds some macros to factor out repetitive code patterns.

The commit also moves the handling of sym::test out of build_configuration into default_configuration, where all the other symbols are handled.

r? @bjorn3

@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 1, 2023
@nnethercote
Copy link
Contributor Author

cc @Urgau

There are some njn: comments in the commit indicating places were the two functions are currently inconsistent.

  • sanitizer_cfi_normalize_{integers,pointers} should probably be added to fill_well_known.
  • I'm not sure why doc, doctest, miri, and target_features are in fill_well_known when they're not in default_configuration.

Any suggestions how to handle these are welcome.


// Target specific values
// njn: sym::sanitizer_cfi_normalize_integers missing!
// njn: sym::sanitizer_cfi_normalize_pointers missing!
Copy link
Member

Choose a reason for hiding this comment

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

These should be added I think.

Copy link
Member

@Urgau Urgau Dec 1, 2023

Choose a reason for hiding this comment

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

@rcvalle What is the story about the configs sanitizer_cfi_normalize_integers and sanitizer_cfi_normalize_pointers ? Are they expected to be used by users ? or just std/core ?

If it's only for core/std, it shouldn't be added, since it's not user-facing.

Copy link
Member

Choose a reason for hiding this comment

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

They're expected to be used by users with the cfi_encoding attribute. For example, they're currently used by the cfi_types crate.

Copy link
Member

@Urgau Urgau Dec 5, 2023

Choose a reason for hiding this comment

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

Right, in that case let's define them like this:

Suggested change
// njn: sym::sanitizer_cfi_normalize_pointers missing!
ins!(sym::sanitizer_cfi_normalize_integers, no_values);
ins!(sym::sanitizer_cfi_generalize_pointers, no_values);

EDIT: Oops, I meant generalize_pointers not normalize_integers. Sorry


Btw, I don't see those cfgs in GATED_CFGS, the list of unstable cfgs. I think they should be added there since they are unstable, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're right. Would you mind also adding them as part of this PR? Thank you!

@nnethercote nnethercote force-pushed the default_configuration-fill_well_known branch from 6da79b5 to afeb3e6 Compare December 4, 2023 23:47
@nnethercote
Copy link
Contributor Author

I have added new comments to the doc/doctest/miri and target_feature cases. I am still unsure what to do with the sanitizer ones.

@bjorn3
Copy link
Member

bjorn3 commented Dec 5, 2023

I am still unsure what to do with the sanitizer ones.

Not sure either.

@Urgau
Copy link
Member

Urgau commented Dec 5, 2023

I am still unsure what to do with the sanitizer ones.

I would leave them for now. I don't think they're meant to be used outside of core, but if they are, we can always add them later.

@rcvalle
Copy link
Member

rcvalle commented Dec 5, 2023

I am still unsure what to do with the sanitizer ones.

I would leave them for now. I don't think they're meant to be used outside of core, but if they are, we can always add them later.

Please keep these as they're expected to be used by users with the cfi_encoding attribute, and they're currently used by the cfi_types crate.

@Urgau
Copy link
Member

Urgau commented Dec 5, 2023

Please keep these as they're expected to be used by users with the cfi_encoding attribute, and they're currently used by the cfi_types crate.

Rest assured that we have no plans to remove them. We were just puzzled by the fact that these cfgs were not defined in the well known check-cfg list.

@rcvalle
Copy link
Member

rcvalle commented Dec 5, 2023

Rest assured that we have no plans to remove them. We were just puzzled by the fact that these cfgs were not defined in the well known check-cfg list.

Thank you! That was probably an oversight on my part when I added them.

@nnethercote nnethercote force-pushed the default_configuration-fill_well_known branch from afeb3e6 to 634ae81 Compare December 5, 2023 20:12
@nnethercote
Copy link
Contributor Author

I have added the sanitizer_cfi ones to fill_well_known.

I haven't touched GATED_CFGS, that would need quite a few changes, e.g. some new cfg symbols defined, tests, and updates to the unstable book. I figure that's scope creep for this PR, but could be done in a follow-up.

@bjorn3: should be ready for final review now, thanks.

@rust-log-analyzer

This comment has been minimized.

compiler/rustc_session/src/config.rs Outdated Show resolved Hide resolved
@nnethercote nnethercote force-pushed the default_configuration-fill_well_known branch from 634ae81 to cf59759 Compare December 5, 2023 20:29
@nnethercote
Copy link
Contributor Author

I pushed an update that fixes the compile error, addresses @Urgau's comment, and updates expected output for the check-cfg tests. Should genuinely be ready for review now :)

@bors
Copy link
Contributor

bors commented Dec 7, 2023

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

@nnethercote
Copy link
Contributor Author

@bjorn3 is low on time right now, so let's try a different reviewer:

r? @Mark-Simulacrum

Note that @Urgau, the resident expert on this code, has given approval, so a light review might suffice. I need to fix the conflicts, but they should just be trivial changes in test output.

@rustbot rustbot assigned Mark-Simulacrum and unassigned bjorn3 Dec 7, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 8, 2023
…own, r=nnethercote

Strengthen well known check-cfg names and values test

rust-lang#118494 is changing the implementation of how we expect well known check-cfg names and values, but we currently don't have a test that checks every well known only some of them.

This PR therefore strengthen our well known names/values test to include all of the configs to at least avoid unintended regressions and validate new entry.

*this PR also contains some drive-by consolidation of unexpected `target_os`, `target_arch` into a single file*

r? `@nnethercote` (maybe? feel free to re-assign)
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 8, 2023
…own, r=nnethercote

Strengthen well known check-cfg names and values test

rust-lang#118494 is changing the implementation of how we expect well known check-cfg names and values, but we currently don't have a test that checks every well known only some of them.

This PR therefore strengthen our well known names/values test to include all of the configs to at least avoid unintended regressions and validate new entry.

*this PR also contains some drive-by consolidation of unexpected `target_os`, `target_arch` into a single file*

r? ``@nnethercote`` (maybe? feel free to re-assign)
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 8, 2023
…own, r=nnethercote

Strengthen well known check-cfg names and values test

rust-lang#118494 is changing the implementation of how we expect well known check-cfg names and values, but we currently don't have a test that checks every well known only some of them.

This PR therefore strengthen our well known names/values test to include all of the configs to at least avoid unintended regressions and validate new entry.

*this PR also contains some drive-by consolidation of unexpected `target_os`, `target_arch` into a single file*

r? ```@nnethercote``` (maybe? feel free to re-assign)
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Dec 9, 2023
…own, r=nnethercote

Strengthen well known check-cfg names and values test

rust-lang#118494 is changing the implementation of how we expect well known check-cfg names and values, but we currently don't have a test that checks every well known only some of them.

This PR therefore strengthen our well known names/values test to include all of the configs to at least avoid unintended regressions and validate new entry.

*this PR also contains some drive-by consolidation of unexpected `target_os`, `target_arch` into a single file*

r? `@nnethercote` (maybe? feel free to re-assign)
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 9, 2023
…own, r=nnethercote

Strengthen well known check-cfg names and values test

rust-lang#118494 is changing the implementation of how we expect well known check-cfg names and values, but we currently don't have a test that checks every well known only some of them.

This PR therefore strengthen our well known names/values test to include all of the configs to at least avoid unintended regressions and validate new entry.

*this PR also contains some drive-by consolidation of unexpected `target_os`, `target_arch` into a single file*

r? `@nnethercote` (maybe? feel free to re-assign)
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

r=me

@Mark-Simulacrum Mark-Simulacrum 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 Dec 9, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2023
Rollup merge of rust-lang#118702 - Urgau:check-cfg-strengthen-well-known, r=nnethercote

Strengthen well known check-cfg names and values test

rust-lang#118494 is changing the implementation of how we expect well known check-cfg names and values, but we currently don't have a test that checks every well known only some of them.

This PR therefore strengthen our well known names/values test to include all of the configs to at least avoid unintended regressions and validate new entry.

*this PR also contains some drive-by consolidation of unexpected `target_os`, `target_arch` into a single file*

r? `@nnethercote` (maybe? feel free to re-assign)
There are comments saying these two functions should be kept in sync,
but they have very different structures, process symbols in different
orders, and there are some inconsistencies.

This commit reorders them so they're both mostly processing symbols in
alphabetical order, which makes cross-checking them a lot easier. The
commit also adds some macros to factor out repetitive code patterns.
Plus it adds `sanitizer_cfi_normalize_{integers,pointers}` to
`fill_well_known`, which were missing.

The commit also moves the handling of `sym::test` out of
`build_configuration` into `default_configuration`, where all the other
symbols are handled.
@nnethercote nnethercote force-pushed the default_configuration-fill_well_known branch from cf59759 to 22b534d Compare December 10, 2023 23:05
@nnethercote
Copy link
Contributor Author

I rebased.

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Dec 10, 2023

📌 Commit 22b534d has been approved by Mark-Simulacrum

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 10, 2023
@bors
Copy link
Contributor

bors commented Dec 11, 2023

⌛ Testing commit 22b534d with merge c13187c...

@bors
Copy link
Contributor

bors commented Dec 11, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing c13187c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 11, 2023
@bors bors merged commit c13187c into rust-lang:master Dec 11, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 11, 2023
@bors bors mentioned this pull request Dec 11, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c13187c): 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

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: 668.003s -> 669.249s (0.19%)
Artifact size: 314.13 MiB -> 314.19 MiB (0.02%)

@nnethercote nnethercote deleted the default_configuration-fill_well_known branch December 11, 2023 21:11
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2023
…aKO8Ki

Add all known `target_feature` configs to check-cfg

This PR adds all the known `target_feature` from ~~`rustc_codegen_ssa`~~ `rustc_target` to the well known list of check-cfg.

It does so by moving the list from `rustc_codegen_ssa` to `rustc_target` ~~`rustc_session` (I not sure about this, but some of the moved function take a `Session`)~~, then using it the `fill_well_known` function.

This already proved to be useful since portable-simd had a bad cfg.

cc `@nnethercote` (since we discussed it in rust-lang#118494)
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Dec 15, 2023
…=TaKO8Ki,GuillaumeGomez,workingjubilee

Add all known `target_feature` configs to check-cfg

This PR adds all the known `target_feature` from ~~`rustc_codegen_ssa`~~ `rustc_target` to the well known list of check-cfg.

It does so by moving the list from `rustc_codegen_ssa` to `rustc_target` ~~`rustc_session` (I not sure about this, but some of the moved function take a `Session`)~~, then using it the `fill_well_known` function.

This already proved to be useful since portable-simd had a bad cfg.

cc `@nnethercote` (since we discussed it in rust-lang#118494)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 15, 2023
Rollup merge of rust-lang#118908 - Urgau:check-cfg-target-features, r=TaKO8Ki,GuillaumeGomez,workingjubilee

Add all known `target_feature` configs to check-cfg

This PR adds all the known `target_feature` from ~~`rustc_codegen_ssa`~~ `rustc_target` to the well known list of check-cfg.

It does so by moving the list from `rustc_codegen_ssa` to `rustc_target` ~~`rustc_session` (I not sure about this, but some of the moved function take a `Session`)~~, then using it the `fill_well_known` function.

This already proved to be useful since portable-simd had a bad cfg.

cc `@nnethercote` (since we discussed it in rust-lang#118494)
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Dec 26, 2023
…er-cfi-cfgs, r=Nilstrieb

Add missing feature gate for sanitizer CFI cfgs

Found during the review of rust-lang#118494 in rust-lang#118494 (comment).

cc `@rcvalle`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 26, 2023
Rollup merge of rust-lang#119235 - Urgau:missing-feature-gate-sanitizer-cfi-cfgs, r=Nilstrieb

Add missing feature gate for sanitizer CFI cfgs

Found during the review of rust-lang#118494 in rust-lang#118494 (comment).

cc `@rcvalle`
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. 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.

9 participants