-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add basic Serde serialization capabilities to Stable MIR #126963
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. This PR changes Stable MIR |
This comment has been minimized.
This comment has been minimized.
Please squash all commits, then this lgtm |
8184e2d
to
414ebea
Compare
Squashed as requested and all tests showing passing results. |
Thanks @bors r+ rollup |
…ive, r=oli-obk Add basic Serde serialization capabilities to Stable MIR This PR adds basic Serde serialization capabilities to Stable MIR. It is intentionally minimal (just wrapping all stable MIR types with a Serde `derive`), so that any important design decisions can be discussed before going further. A simple test is included with this PR to validate that JSON can actually be emitted. ## Notes When I wrapped the Stable MIR error types in `compiler/stable_mir/src/error.rs`, it caused test failures (though I'm not sure why) so I backed those out. ## Future Work So, this PR will support serializing basic stable MIR, but it _does not_ support serializing interned values beneath `Ty`s and `AllocId`s, etc... My current thinking about how to handle this is as follows: 1. Add new `visited_X` fields to the `Tables` struct for each interned category of interest. 2. As serialization is occuring, serialize interned values as usual _and_ also record the interned value we referenced in `visited_X`. (Possibly) In addition, if an interned value recursively references other interned values, record those interned values as well. 3. Teach the stable MIR `Context` how to access the `visited_X` values and expose them with wrappers in `stable_mir/src/lib.rs` to users (e.g. to serialize and/or further analyze them). ### Pros This approach does not commit to any specific serialization format regarding interned values or other more complex cases, which avoids us locking into any behaviors that may not be desired long-term. ### Cons The user will need to manually handle serializing interned values. ### Alternatives 1. We can directly provide access to the underlying `Tables` maps for interned values; the disadvantage of this approach is that it either requires extra processing for users to filter out to only use the values that they need _or_ users may serialize extra values that they don't need. The advantage is that the implementation is even simpler. The other pros/cons are similar to the above. 2. We can directly serialize interned values by expanding them in-place. The pro is that this may make some basic inputs easier to consume. However, the cons are that there will need to be special provisions for dealing with cyclical values on both the producer and consumer _and_ global values will possibly need to be de-duplicated on the consumer side.
…llaumeGomez Rollup of 10 pull requests Successful merges: - rust-lang#123237 (Various rustc_codegen_ssa cleanups) - rust-lang#123714 (Add test for fn pointer duplication.) - rust-lang#124091 (Update AST validation module docs) - rust-lang#126835 (Simplifications in match lowering) - rust-lang#126963 (Add basic Serde serialization capabilities to Stable MIR) - rust-lang#127010 (Update browser-ui-test version to `0.18.0`) - rust-lang#127015 (Switch back `non_local_definitions` lint to allow-by-default) - rust-lang#127016 (docs: check if the disambiguator matches its suffix) - rust-lang#127029 (Fix Markdown tables in platform-support.md) - rust-lang#127032 (Enable const casting for `f16` and `f128`) r? `@ghost` `@rustbot` modify labels: rollup
…ive, r=oli-obk Add basic Serde serialization capabilities to Stable MIR This PR adds basic Serde serialization capabilities to Stable MIR. It is intentionally minimal (just wrapping all stable MIR types with a Serde `derive`), so that any important design decisions can be discussed before going further. A simple test is included with this PR to validate that JSON can actually be emitted. ## Notes When I wrapped the Stable MIR error types in `compiler/stable_mir/src/error.rs`, it caused test failures (though I'm not sure why) so I backed those out. ## Future Work So, this PR will support serializing basic stable MIR, but it _does not_ support serializing interned values beneath `Ty`s and `AllocId`s, etc... My current thinking about how to handle this is as follows: 1. Add new `visited_X` fields to the `Tables` struct for each interned category of interest. 2. As serialization is occuring, serialize interned values as usual _and_ also record the interned value we referenced in `visited_X`. (Possibly) In addition, if an interned value recursively references other interned values, record those interned values as well. 3. Teach the stable MIR `Context` how to access the `visited_X` values and expose them with wrappers in `stable_mir/src/lib.rs` to users (e.g. to serialize and/or further analyze them). ### Pros This approach does not commit to any specific serialization format regarding interned values or other more complex cases, which avoids us locking into any behaviors that may not be desired long-term. ### Cons The user will need to manually handle serializing interned values. ### Alternatives 1. We can directly provide access to the underlying `Tables` maps for interned values; the disadvantage of this approach is that it either requires extra processing for users to filter out to only use the values that they need _or_ users may serialize extra values that they don't need. The advantage is that the implementation is even simpler. The other pros/cons are similar to the above. 2. We can directly serialize interned values by expanding them in-place. The pro is that this may make some basic inputs easier to consume. However, the cons are that there will need to be special provisions for dealing with cyclical values on both the producer and consumer _and_ global values will possibly need to be de-duplicated on the consumer side.
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#123237 (Various rustc_codegen_ssa cleanups) - rust-lang#123714 (Add test for fn pointer duplication.) - rust-lang#124091 (Update AST validation module docs) - rust-lang#126835 (Simplifications in match lowering) - rust-lang#126963 (Add basic Serde serialization capabilities to Stable MIR) - rust-lang#127010 (Update browser-ui-test version to `0.18.0`) - rust-lang#127015 (Switch back `non_local_definitions` lint to allow-by-default) - rust-lang#127029 (Fix Markdown tables in platform-support.md) - rust-lang#127032 (Enable const casting for `f16` and `f128`) r? `@ghost` `@rustbot` modify labels: rollup
…ive, r=oli-obk Add basic Serde serialization capabilities to Stable MIR This PR adds basic Serde serialization capabilities to Stable MIR. It is intentionally minimal (just wrapping all stable MIR types with a Serde `derive`), so that any important design decisions can be discussed before going further. A simple test is included with this PR to validate that JSON can actually be emitted. ## Notes When I wrapped the Stable MIR error types in `compiler/stable_mir/src/error.rs`, it caused test failures (though I'm not sure why) so I backed those out. ## Future Work So, this PR will support serializing basic stable MIR, but it _does not_ support serializing interned values beneath `Ty`s and `AllocId`s, etc... My current thinking about how to handle this is as follows: 1. Add new `visited_X` fields to the `Tables` struct for each interned category of interest. 2. As serialization is occuring, serialize interned values as usual _and_ also record the interned value we referenced in `visited_X`. (Possibly) In addition, if an interned value recursively references other interned values, record those interned values as well. 3. Teach the stable MIR `Context` how to access the `visited_X` values and expose them with wrappers in `stable_mir/src/lib.rs` to users (e.g. to serialize and/or further analyze them). ### Pros This approach does not commit to any specific serialization format regarding interned values or other more complex cases, which avoids us locking into any behaviors that may not be desired long-term. ### Cons The user will need to manually handle serializing interned values. ### Alternatives 1. We can directly provide access to the underlying `Tables` maps for interned values; the disadvantage of this approach is that it either requires extra processing for users to filter out to only use the values that they need _or_ users may serialize extra values that they don't need. The advantage is that the implementation is even simpler. The other pros/cons are similar to the above. 2. We can directly serialize interned values by expanding them in-place. The pro is that this may make some basic inputs easier to consume. However, the cons are that there will need to be special provisions for dealing with cyclical values on both the producer and consumer _and_ global values will possibly need to be de-duplicated on the consumer side.
…ive, r=oli-obk Add basic Serde serialization capabilities to Stable MIR This PR adds basic Serde serialization capabilities to Stable MIR. It is intentionally minimal (just wrapping all stable MIR types with a Serde `derive`), so that any important design decisions can be discussed before going further. A simple test is included with this PR to validate that JSON can actually be emitted. ## Notes When I wrapped the Stable MIR error types in `compiler/stable_mir/src/error.rs`, it caused test failures (though I'm not sure why) so I backed those out. ## Future Work So, this PR will support serializing basic stable MIR, but it _does not_ support serializing interned values beneath `Ty`s and `AllocId`s, etc... My current thinking about how to handle this is as follows: 1. Add new `visited_X` fields to the `Tables` struct for each interned category of interest. 2. As serialization is occuring, serialize interned values as usual _and_ also record the interned value we referenced in `visited_X`. (Possibly) In addition, if an interned value recursively references other interned values, record those interned values as well. 3. Teach the stable MIR `Context` how to access the `visited_X` values and expose them with wrappers in `stable_mir/src/lib.rs` to users (e.g. to serialize and/or further analyze them). ### Pros This approach does not commit to any specific serialization format regarding interned values or other more complex cases, which avoids us locking into any behaviors that may not be desired long-term. ### Cons The user will need to manually handle serializing interned values. ### Alternatives 1. We can directly provide access to the underlying `Tables` maps for interned values; the disadvantage of this approach is that it either requires extra processing for users to filter out to only use the values that they need _or_ users may serialize extra values that they don't need. The advantage is that the implementation is even simpler. The other pros/cons are similar to the above. 2. We can directly serialize interned values by expanding them in-place. The pro is that this may make some basic inputs easier to consume. However, the cons are that there will need to be special provisions for dealing with cyclical values on both the producer and consumer _and_ global values will possibly need to be de-duplicated on the consumer side.
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#123237 (Various rustc_codegen_ssa cleanups) - rust-lang#123714 (Add test for fn pointer duplication.) - rust-lang#124091 (Update AST validation module docs) - rust-lang#126835 (Simplifications in match lowering) - rust-lang#126963 (Add basic Serde serialization capabilities to Stable MIR) - rust-lang#127015 (Switch back `non_local_definitions` lint to allow-by-default) - rust-lang#127029 (Fix Markdown tables in platform-support.md) - rust-lang#127032 (Enable const casting for `f16` and `f128`) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#123237 (Various rustc_codegen_ssa cleanups) - rust-lang#123714 (Add test for fn pointer duplication.) - rust-lang#124091 (Update AST validation module docs) - rust-lang#126835 (Simplifications in match lowering) - rust-lang#126963 (Add basic Serde serialization capabilities to Stable MIR) - rust-lang#127015 (Switch back `non_local_definitions` lint to allow-by-default) - rust-lang#127029 (Fix Markdown tables in platform-support.md) - rust-lang#127032 (Enable const casting for `f16` and `f128`) r? `@ghost` `@rustbot` modify labels: rollup
…ive, r=oli-obk Add basic Serde serialization capabilities to Stable MIR This PR adds basic Serde serialization capabilities to Stable MIR. It is intentionally minimal (just wrapping all stable MIR types with a Serde `derive`), so that any important design decisions can be discussed before going further. A simple test is included with this PR to validate that JSON can actually be emitted. ## Notes When I wrapped the Stable MIR error types in `compiler/stable_mir/src/error.rs`, it caused test failures (though I'm not sure why) so I backed those out. ## Future Work So, this PR will support serializing basic stable MIR, but it _does not_ support serializing interned values beneath `Ty`s and `AllocId`s, etc... My current thinking about how to handle this is as follows: 1. Add new `visited_X` fields to the `Tables` struct for each interned category of interest. 2. As serialization is occuring, serialize interned values as usual _and_ also record the interned value we referenced in `visited_X`. (Possibly) In addition, if an interned value recursively references other interned values, record those interned values as well. 3. Teach the stable MIR `Context` how to access the `visited_X` values and expose them with wrappers in `stable_mir/src/lib.rs` to users (e.g. to serialize and/or further analyze them). ### Pros This approach does not commit to any specific serialization format regarding interned values or other more complex cases, which avoids us locking into any behaviors that may not be desired long-term. ### Cons The user will need to manually handle serializing interned values. ### Alternatives 1. We can directly provide access to the underlying `Tables` maps for interned values; the disadvantage of this approach is that it either requires extra processing for users to filter out to only use the values that they need _or_ users may serialize extra values that they don't need. The advantage is that the implementation is even simpler. The other pros/cons are similar to the above. 2. We can directly serialize interned values by expanding them in-place. The pro is that this may make some basic inputs easier to consume. However, the cons are that there will need to be special provisions for dealing with cyclical values on both the producer and consumer _and_ global values will possibly need to be de-duplicated on the consumer side.
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#123237 (Various rustc_codegen_ssa cleanups) - rust-lang#123714 (Add test for fn pointer duplication.) - rust-lang#124091 (Update AST validation module docs) - rust-lang#126835 (Simplifications in match lowering) - rust-lang#126963 (Add basic Serde serialization capabilities to Stable MIR) - rust-lang#127015 (Switch back `non_local_definitions` lint to allow-by-default) - rust-lang#127029 (Fix Markdown tables in platform-support.md) - rust-lang#127032 (Enable const casting for `f16` and `f128`) r? `@ghost` `@rustbot` modify labels: rollup
…llaumeGomez Rollup of 10 pull requests Successful merges: - rust-lang#123714 (Add test for fn pointer duplication.) - rust-lang#124091 (Update AST validation module docs) - rust-lang#126963 (Add basic Serde serialization capabilities to Stable MIR) - rust-lang#127015 (Switch back `non_local_definitions` lint to allow-by-default) - rust-lang#127016 (docs: check if the disambiguator matches its suffix) - rust-lang#127029 (Fix Markdown tables in platform-support.md) - rust-lang#127032 (Enable const casting for `f16` and `f128`) - rust-lang#127041 (Migrate `run-make/override-aliased-flags` to `rmake.rs`) - rust-lang#127045 (Rename `super_predicates_of` and similar queries to `explicit_*` to note that they're not elaborated) - rust-lang#127075 (rustc_data_structures: Explicitly check for 64-bit atomics support) r? `@ghost` `@rustbot` modify labels: rollup
@bors r+ rollup=iffy that failure looks like a miri failure |
…e, r=oli-obk Add basic Serde serialization capabilities to Stable MIR This PR adds basic Serde serialization capabilities to Stable MIR. It is intentionally minimal (just wrapping all stable MIR types with a Serde `derive`), so that any important design decisions can be discussed before going further. A simple test is included with this PR to validate that JSON can actually be emitted. ## Notes When I wrapped the Stable MIR error types in `compiler/stable_mir/src/error.rs`, it caused test failures (though I'm not sure why) so I backed those out. ## Future Work So, this PR will support serializing basic stable MIR, but it _does not_ support serializing interned values beneath `Ty`s and `AllocId`s, etc... My current thinking about how to handle this is as follows: 1. Add new `visited_X` fields to the `Tables` struct for each interned category of interest. 2. As serialization is occuring, serialize interned values as usual _and_ also record the interned value we referenced in `visited_X`. (Possibly) In addition, if an interned value recursively references other interned values, record those interned values as well. 3. Teach the stable MIR `Context` how to access the `visited_X` values and expose them with wrappers in `stable_mir/src/lib.rs` to users (e.g. to serialize and/or further analyze them). ### Pros This approach does not commit to any specific serialization format regarding interned values or other more complex cases, which avoids us locking into any behaviors that may not be desired long-term. ### Cons The user will need to manually handle serializing interned values. ### Alternatives 1. We can directly provide access to the underlying `Tables` maps for interned values; the disadvantage of this approach is that it either requires extra processing for users to filter out to only use the values that they need _or_ users may serialize extra values that they don't need. The advantage is that the implementation is even simpler. The other pros/cons are similar to the above. 2. We can directly serialize interned values by expanding them in-place. The pro is that this may make some basic inputs easier to consume. However, the cons are that there will need to be special provisions for dealing with cyclical values on both the producer and consumer _and_ global values will possibly need to be de-duplicated on the consumer side.
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#123237 (Various rustc_codegen_ssa cleanups) - rust-lang#123714 (Add test for fn pointer duplication.) - rust-lang#124091 (Update AST validation module docs) - rust-lang#126835 (Simplifications in match lowering) - rust-lang#126963 (Add basic Serde serialization capabilities to Stable MIR) - rust-lang#127015 (Switch back `non_local_definitions` lint to allow-by-default) - rust-lang#127029 (Fix Markdown tables in platform-support.md) - rust-lang#127032 (Enable const casting for `f16` and `f128`) r? `@ghost` `@rustbot` modify labels: rollup
@oli-obk Thanks again for your review! I have two questions about the status of this PR:
Thanks much! CI Failure Log NotesFrom reading the CI failure log, it looks like the As far as I can tell, this is either a sporadic CI issue or related to a different PR in the roll-up. I'd love to know your thoughts!
|
@bors r- bors sync |
it seems likely considering they occurred twice, but I don't really know how that can happen. Try running miri tests locally to see if it reproduces |
The failure seems specific to |
I really don't see how this PR could be breaking CI. I also noticed that different runs presented similar errors (failed to copy an artifact because it is currently being used), however, with different artifacts. Also, this PR (#127096) was created without this change, and it had similar failure: #127096 (comment). A retry seemed to have fixed the issue. @bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (7120fda): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis 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. CyclesResults (secondary 7.1%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 771.494s -> 770.291s (-0.16%) |
@celinval Thanks for re-running this! Glad to get this merged so we can have an easy-to-consume representation of the stable MIR AST! |
This PR adds basic Serde serialization capabilities to Stable MIR. It is intentionally minimal (just wrapping all stable MIR types with a Serde
derive
), so that any important design decisions can be discussed before going further. A simple test is included with this PR to validate that JSON can actually be emitted.Notes
When I wrapped the Stable MIR error types in
compiler/stable_mir/src/error.rs
, it caused test failures (though I'm not sure why) so I backed those out.Future Work
So, this PR will support serializing basic stable MIR, but it does not support serializing interned values beneath
Ty
s andAllocId
s, etc... My current thinking about how to handle this is as follows:Add new
visited_X
fields to theTables
struct for each interned category of interest.As serialization is occuring, serialize interned values as usual and also record the interned value we referenced in
visited_X
.(Possibly) In addition, if an interned value recursively references other interned values, record those interned values as well.
Teach the stable MIR
Context
how to access thevisited_X
values and expose them with wrappers instable_mir/src/lib.rs
to users (e.g. to serialize and/or further analyze them).Pros
This approach does not commit to any specific serialization format regarding interned values or other more complex cases, which avoids us locking into any behaviors that may not be desired long-term.
Cons
The user will need to manually handle serializing interned values.
Alternatives
We can directly provide access to the underlying
Tables
maps for interned values; the disadvantage of this approach is that it either requires extra processing for users to filter out to only use the values that they need or users may serialize extra values that they don't need. The advantage is that the implementation is even simpler. The other pros/cons are similar to the above.We can directly serialize interned values by expanding them in-place. The pro is that this may make some basic inputs easier to consume. However, the cons are that there will need to be special provisions for dealing with cyclical values on both the producer and consumer and global values will possibly need to be de-duplicated on the consumer side.