Refactor some `std` code that works with pointer offstes #100823

WaffleLapkin · 2022-08-20T23:07:48Z

This PR replaces pointer::offset in standard library with pointer::add and pointer::sub, [re]moving some casts and using .addr() while we are at it.

This is a more complicated refactor than all other sibling PRs, so take a closer look when reviewing, please 😃 (though I've checked this multiple times and it looks fine).

r? @scottmcm

split off from #100746, continuation of #100822

rustbot · 2022-08-20T23:07:51Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

bors · 2022-08-29T23:35:04Z

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

…ls/alloc.rs` nicer - Use `.addr()` instead of `as`-cast - Use `add` instead of `offset` and remove some `as isize` casts by doing that - Remove some casts

WaffleLapkin · 2022-09-04T15:11:56Z

Rebased to fix merge conflict. r? @scottmcm

scottmcm · 2022-09-06T05:57:26Z

library/core/src/fmt/num.rs

            let buf_ptr = MaybeUninit::slice_as_mut_ptr(&mut buf);
            let lut_ptr = DEC_DIGITS_LUT.as_ptr();

            // decode 2 chars at a time
            while n >= 100 {
-                let d1 = ((n % 100) as isize) << 1;
+                let d1 = ((n % 100) << 1) as usize;


For safety, can you switch this to be more similar to the old one?

Suggested change

let d1 = ((n % 100) << 1) as usize;

let d1 = ((n % 100) as usize) << 1;

The macro defining this function is instantiated with i8 (

rust/library/core/src/fmt/num.rs

Lines 465 to 468 in 3ea4493

impl_Exp!(

i8, u8, i16, u16, i32, u32, i64, u64, usize, isize

as u64 via to_u64 named exp_u64

);

), meaning that n could be i8, so if n was 99_i8 then the shift would overflow if it's done before the cast.

(Can that ever happen? No idea. Up to you if you want to try to write a test to trigger it somehow. But I think it's worth changing the code to obviously not be a problem just in case.)

Oh, wait, no, we only get here if n >= 100, but (127_i8 % 100) << 1, so I guess it'd be fine.

That's still a bit too subtle for my taste, though, so I'd rather just shift in the bigger type.

scottmcm · 2022-09-06T06:07:14Z

library/core/src/fmt/num.rs

        }
        // `n` < 1e8 < (1 << 32)
        let mut n = n as u32;
        if n >= 1e4 as u32 {
            let to_parse = n % 1e4 as u32;
            n /= 1e4 as u32;

-            let d1 = (to_parse / 100) << 1;
-            let d2 = (to_parse % 100) << 1;
+            let d1 = ((to_parse / 100) << 1) as usize;


pondering: I'm not sure if moving the casts up to the dN variables helps. Based on the structure hwere, where these various blocks are working in particular bitwidths based on the range of n, I think it's kinda nice to have then dN variables still be in those types. Which would then imply keeping the cast in the pointer operations, just as .add(d1 as usize) instead of offset+isize. How do you feel about that? I'm not sure how strongly I feel here, so absolutely feel free to argue either way 🙃

(The curr being usize is definitely a nice change, though.)

I actually agree, dN are only used once, so it doesn't really matter where the cast is, and with the cast in add it looks nicer.

scottmcm · 2022-09-06T06:09:28Z

library/core/src/fmt/num.rs

        } else {
-            let d1 = n << 1;
+            let d1 = (n << 1) as usize;


aside: I guess this one is also overflowing when n is 99_i8, even before your change, but it just overflows into the sign bit which I guess doesn't matter in the end after the casts?

n is cast to u16 on line 548, so no overflows here :)

Overflow into a sign bit would be pretty bad:

[src/main.rs:2] 99i8 << 1 = -58 [src/main.rs:2] (99i8 << 1) as usize = 18446744073709551558

scottmcm · 2022-09-06T06:11:10Z

library/std/src/sys/sgx/abi/usercalls/alloc.rs

@@ -316,9 +316,9 @@ where
 //   | small1 | Chunk smaller than 8 bytes
 //   +--------+
 fn region_as_aligned_chunks(ptr: *const u8, len: usize) -> (usize, usize, usize) {


pondering (not this PR): this looks an awful lot like .align_to(8).

library/std/src/sys/sgx/abi/usercalls/alloc.rs

scottmcm · 2022-09-22T23:55:39Z

Looks good!

@bors r+

And thanks again for splitting out the various parts of this.

bors · 2022-09-22T23:55:41Z

📌 Commit 98a3230 has been approved by scottmcm

It is now in the queue for this repository.

Refactor some `std` code that works with pointer offstes This PR replaces `pointer::offset` in standard library with `pointer::add` and `pointer::sub`, [re]moving some casts and using `.addr()` while we are at it. This is a more complicated refactor than all other sibling PRs, so take a closer look when reviewing, please 😃 (though I've checked this multiple times and it looks fine). r? ``@scottmcm`` _split off from rust-lang#100746, continuation of #100822_

Refactor some `std` code that works with pointer offstes This PR replaces `pointer::offset` in standard library with `pointer::add` and `pointer::sub`, [re]moving some casts and using `.addr()` while we are at it. This is a more complicated refactor than all other sibling PRs, so take a closer look when reviewing, please 😃 (though I've checked this multiple times and it looks fine). r? ```@scottmcm``` _split off from rust-lang#100746, continuation of #100822_

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#100823 (Refactor some `std` code that works with pointer offstes) - rust-lang#102088 (Fix wrongly refactored Lift impl) - rust-lang#102109 (resolve: Set effective visibilities for imports more precisely) - rust-lang#102186 (Add const_closure, Constify Try trait) - rust-lang#102203 (rustdoc: remove no-op CSS `#source-sidebar { z-index }`) - rust-lang#102204 (Make `ManuallyDrop` satisfy `~const Destruct`) - rust-lang#102210 (diagnostics: avoid syntactically invalid suggestion in if conditionals) - rust-lang#102226 (bootstrap/miri: switch to non-deprecated env var for setting the sysroot folder) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned scottmcm Aug 20, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 20, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2022

WaffleLapkin added 3 commits September 4, 2022 17:27

Make code worling w/ pointers in `library/std/src/sys/sgx/abi/usercal…

a2cdbf8

…ls/alloc.rs` nicer - Use `.addr()` instead of `as`-cast - Use `add` instead of `offset` and remove some `as isize` casts by doing that - Remove some casts

use pointer::add in memchr impl

495fa48

Replace offset with add in fmt/num.rs & remove some casts

31b7181

WaffleLapkin force-pushed the less_offsets branch from ac3455e to 31b7181 Compare September 4, 2022 13:36

scottmcm reviewed Sep 6, 2022

View reviewed changes

library/std/src/sys/sgx/abi/usercalls/alloc.rs Show resolved Hide resolved

scottmcm 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 Sep 6, 2022

Apply changes proposed in the review

98a3230

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 Sep 22, 2022

Dylan-DPC mentioned this pull request Sep 24, 2022

Rollup of 8 pull requests #102222

Closed

Dylan-DPC mentioned this pull request Sep 24, 2022

Rollup of 6 pull requests #102230

Closed

matthiaskrgr mentioned this pull request Sep 24, 2022

Rollup of 8 pull requests #102234

Merged

bors merged commit 1b1596c into rust-lang:master Sep 24, 2022

rustbot added this to the 1.66.0 milestone Sep 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor some `std` code that works with pointer offstes #100823

Refactor some `std` code that works with pointer offstes #100823

WaffleLapkin commented Aug 20, 2022

rustbot commented Aug 20, 2022

bors commented Aug 29, 2022

WaffleLapkin commented Sep 4, 2022

scottmcm Sep 6, 2022

scottmcm Sep 6, 2022

scottmcm Sep 6, 2022

WaffleLapkin Sep 22, 2022

scottmcm Sep 6, 2022

WaffleLapkin Sep 22, 2022

scottmcm Sep 6, 2022

scottmcm commented Sep 22, 2022

bors commented Sep 22, 2022

	let d1 = ((n % 100) << 1) as usize;
	let d1 = ((n % 100) as usize) << 1;

	impl_Exp!(
	i8, u8, i16, u16, i32, u32, i64, u64, usize, isize
	as u64 via to_u64 named exp_u64
	);

Refactor some std code that works with pointer offstes #100823

Refactor some std code that works with pointer offstes #100823

Conversation

WaffleLapkin commented Aug 20, 2022

rustbot commented Aug 20, 2022

bors commented Aug 29, 2022

WaffleLapkin commented Sep 4, 2022

scottmcm Sep 6, 2022

Choose a reason for hiding this comment

scottmcm Sep 6, 2022

Choose a reason for hiding this comment

scottmcm Sep 6, 2022

Choose a reason for hiding this comment

WaffleLapkin Sep 22, 2022

Choose a reason for hiding this comment

scottmcm Sep 6, 2022

Choose a reason for hiding this comment

WaffleLapkin Sep 22, 2022

Choose a reason for hiding this comment

scottmcm Sep 6, 2022

Choose a reason for hiding this comment

scottmcm commented Sep 22, 2022

bors commented Sep 22, 2022

Refactor some `std` code that works with pointer offstes #100823

Refactor some `std` code that works with pointer offstes #100823