Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… #133861

shamb0 · 2024-12-04T15:53:48Z

This PR introduces a new allocate_bytes function in InterpCx and refactors allocate_str to use it internally. This change improves memory allocation handling in the interpreter by:

Adding allocate_bytes:
- Direct byte slice allocation support
- Handles both mutable and immutable allocations
- Maintains proper memory alignment and deduplication
Refactoring allocate_str:
- Now uses allocate_bytes internally
- Adds string-specific metadata handling
- Preserves existing string allocation behavior

This is part 1 of the planned changes to improve timezone string handling in Miri. A follow-up PR will update Miri's timezone logic to use this new allocation mechanism.

Related: rust-lang/miri#4069

rustbot · 2024-12-04T15:53:57Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

RalfJung · 2024-12-04T16:39:06Z

r? @RalfJung

rustbot · 2024-12-05T02:10:11Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

RalfJung

Thanks for the PR! I have a few minor comments.

RalfJung · 2024-12-05T07:16:47Z

compiler/rustc_const_eval/src/interpret/place.rs

        let ptr = if mutbl.is_not() {
-            // Use dedup'd allocation function.
+            // Deduplicate immutable allocations using a salted hash


You have changed all the comments in this function. The new comments aren't wrong, but they are also not clearly better than the old ones. Why have you rewritten them?
The salted hash part isn't relevant here, what is relevant is that we use the dedup function, so for this line here please restore the previous comment.

When your comment is a sentence, please make sure to end it in ..

Ah, Apologies for my earlier misunderstanding based on the function name. I’ve restored the original comments as per your feedback.

RalfJung · 2024-12-05T07:18:46Z

compiler/rustc_const_eval/src/interpret/place.rs

        &mut self,
-        str: &str,
+        bytes: &[u8],
+        align: Align,
        kind: MemoryKind<M::MemoryKind>,
        mutbl: Mutability,
    ) -> InterpResult<'tcx, MPlaceTy<'tcx, M::Provenance>> {


I think returning this as a Pointer makes more sense, given that we don't know which type the caller needs.

I agree that returning InterpResult<'tcx, Pointer<M::Provenance>> as a pointer fits this context better. Great suggestion!

RalfJung · 2024-12-05T07:36:54Z

@rustbot author

Once the PR is ready for review again, please write @rustbot ready.

shamb0 · 2024-12-05T11:39:32Z

Thank you for taking the time to review and provide comments. I've addressed them all, and I hope the changes are now ready for intake.

shamb0 · 2024-12-05T11:39:54Z

@rustbot ready

RalfJung

Just one final nit. :) And then please squash the PR into a single commit.

compiler/rustc_const_eval/src/interpret/place.rs

RalfJung · 2024-12-05T21:31:33Z

@rustbot author

shamb0 · 2024-12-06T08:16:17Z

@rustbot ready

RalfJung · 2024-12-06T20:54:31Z

You skipped the "squashing into a single commit" part. :)
See https://rustc-dev-guide.rust-lang.org/git.html#squash-your-commits for some help with how to do that.

@rustbot author

… allocation Signed-off-by: shamb0 <r.raajey@gmail.com>

shamb0 · 2024-12-07T11:24:12Z

Apologies for missing this step—I overlooked it while managing multiple tasks. I truly appreciate your patience and dedication to maintaining quality. I’ve now understood the workflow and will ensure this doesn’t happen again in the future. Thank you! :)

shamb0 · 2024-12-07T11:24:46Z

@rustbot ready

RalfJung · 2024-12-07T11:59:09Z

No worries, and thanks for swiftly dealing with all my feedback. :)

@bors r+ rollup

bors · 2024-12-07T11:59:12Z

📌 Commit 9b07e75 has been approved by RalfJung

It is now in the queue for this repository.

…str, r=RalfJung Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… Fixes rust-lang/miri#4025 This PR introduces a new `allocate_bytes` function in InterpCx and refactors `allocate_str` to use it internally. This change improves memory allocation handling in the interpreter by: 1. Adding `allocate_bytes`: - Direct byte slice allocation support - Handles both mutable and immutable allocations - Maintains proper memory alignment and deduplication 2. Refactoring `allocate_str`: - Now uses `allocate_bytes` internally - Adds string-specific metadata handling - Preserves existing string allocation behavior This is part 1 of the planned changes to improve timezone string handling in Miri. A follow-up PR will update Miri's timezone logic to use this new allocation mechanism. Related: rust-lang/miri#4069

…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#133522 (Don't suggest restricting bound with unstable traits on stable and mention it's unstable on nightly) - rust-lang#133733 ( compiletest: show the difference between the normalized output and the actual output for lines which didn't match) - rust-lang#133861 (Add allocate_bytes and refactor allocate_str in InterpCx for raw byte…) - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) - rust-lang#133987 (Define acronym for thread local storage) - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`) - rust-lang#133993 (Fix: typo in E0751 error explanation) - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`) r? `@ghost` `@rustbot` modify labels: rollup

…str, r=RalfJung Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… Fixes rust-lang/miri#4025 This PR introduces a new `allocate_bytes` function in InterpCx and refactors `allocate_str` to use it internally. This change improves memory allocation handling in the interpreter by: 1. Adding `allocate_bytes`: - Direct byte slice allocation support - Handles both mutable and immutable allocations - Maintains proper memory alignment and deduplication 2. Refactoring `allocate_str`: - Now uses `allocate_bytes` internally - Adds string-specific metadata handling - Preserves existing string allocation behavior This is part 1 of the planned changes to improve timezone string handling in Miri. A follow-up PR will update Miri's timezone logic to use this new allocation mechanism. Related: rust-lang/miri#4069

[TEST] Rollup of 5 pull requests Successful merges: - rust-lang#133733 ( compiletest: show the difference between the normalized output and the actual output for lines which didn't match) - rust-lang#133861 (Add allocate_bytes and refactor allocate_str in InterpCx for raw byte…) - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) r? `@ghost` `@rustbot` modify labels: rollup try-job: i686-mingw

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#131669 (lint: change help for pointers to dyn types in FFI) - rust-lang#133104 (crashes: add test for rust-lang#131451) - rust-lang#133767 (Add more info on type/trait mismatches for different crate versions) - rust-lang#133861 (Add allocate_bytes and refactor allocate_str in InterpCx for raw byte…) - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md) - rust-lang#133987 (Define acronym for thread local storage) - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#133861 - shamb0:refactor_InterpCx_allocate_str, r=RalfJung Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… Fixes rust-lang/miri#4025 This PR introduces a new `allocate_bytes` function in InterpCx and refactors `allocate_str` to use it internally. This change improves memory allocation handling in the interpreter by: 1. Adding `allocate_bytes`: - Direct byte slice allocation support - Handles both mutable and immutable allocations - Maintains proper memory alignment and deduplication 2. Refactoring `allocate_str`: - Now uses `allocate_bytes` internally - Adds string-specific metadata handling - Preserves existing string allocation behavior This is part 1 of the planned changes to improve timezone string handling in Miri. A follow-up PR will update Miri's timezone logic to use this new allocation mechanism. Related: rust-lang/miri#4069

rustbot assigned estebank Dec 4, 2024

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 4, 2024

shamb0 mentioned this pull request Dec 4, 2024

localtime_r: deduplicate timezone name allocation rust-lang/miri#4069

Merged

rustbot assigned RalfJung and unassigned estebank Dec 4, 2024

shamb0 marked this pull request as ready for review December 5, 2024 02:10

RalfJung reviewed Dec 5, 2024

View reviewed changes

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 Dec 5, 2024

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 Dec 5, 2024

RalfJung reviewed Dec 5, 2024

View reviewed changes

compiler/rustc_const_eval/src/interpret/place.rs Outdated Show resolved Hide resolved

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 Dec 5, 2024

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 Dec 6, 2024

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 Dec 6, 2024

Add allocate_bytes and refactor allocate_str in InterpCx for raw byte…

9b07e75

… allocation Signed-off-by: shamb0 <r.raajey@gmail.com>

shamb0 force-pushed the refactor_InterpCx_allocate_str branch from e4de6a4 to 9b07e75 Compare December 7, 2024 11:22

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 Dec 7, 2024

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 7, 2024

workingjubilee mentioned this pull request Dec 8, 2024

Rollup of 10 pull requests #134019

Closed

workingjubilee mentioned this pull request Dec 8, 2024

[TEST] Rollup of 5 pull requests #134025

Draft

matthiaskrgr mentioned this pull request Dec 8, 2024

Rollup of 7 pull requests #134033

Merged

bors merged commit 0d26662 into rust-lang:master Dec 8, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… #133861

Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… #133861

shamb0 commented Dec 4, 2024

rustbot commented Dec 4, 2024

RalfJung commented Dec 4, 2024

rustbot commented Dec 5, 2024

RalfJung left a comment

RalfJung Dec 5, 2024

shamb0 Dec 5, 2024

RalfJung Dec 5, 2024

shamb0 Dec 5, 2024

RalfJung commented Dec 5, 2024

shamb0 commented Dec 5, 2024

shamb0 commented Dec 5, 2024

RalfJung left a comment

RalfJung commented Dec 5, 2024

shamb0 commented Dec 6, 2024

RalfJung commented Dec 6, 2024

shamb0 commented Dec 7, 2024

shamb0 commented Dec 7, 2024

RalfJung commented Dec 7, 2024

bors commented Dec 7, 2024

Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… #133861

Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… #133861

Conversation

shamb0 commented Dec 4, 2024

rustbot commented Dec 4, 2024

RalfJung commented Dec 4, 2024

rustbot commented Dec 5, 2024

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung Dec 5, 2024

Choose a reason for hiding this comment

shamb0 Dec 5, 2024

Choose a reason for hiding this comment

RalfJung Dec 5, 2024

Choose a reason for hiding this comment

shamb0 Dec 5, 2024

Choose a reason for hiding this comment

RalfJung commented Dec 5, 2024

shamb0 commented Dec 5, 2024

shamb0 commented Dec 5, 2024

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Dec 5, 2024

shamb0 commented Dec 6, 2024

RalfJung commented Dec 6, 2024

shamb0 commented Dec 7, 2024

shamb0 commented Dec 7, 2024

RalfJung commented Dec 7, 2024

bors commented Dec 7, 2024