From 6eb76c70f67ffd75e8083075eb9732dd5974971f Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Thu, 28 Apr 2022 18:49:45 -0700 Subject: [PATCH 1/3] Work around bad `ptr::with_addr` codegen in `core::ptr` --- library/core/src/ptr/const_ptr.rs | 20 ++++++++++++++------ library/core/src/ptr/mut_ptr.rs | 20 ++++++++++++++------ 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/library/core/src/ptr/const_ptr.rs b/library/core/src/ptr/const_ptr.rs index 7ef2e95542bba..f0cfd27027dda 100644 --- a/library/core/src/ptr/const_ptr.rs +++ b/library/core/src/ptr/const_ptr.rs @@ -242,12 +242,20 @@ impl *const T { // In the mean-time, this operation is defined to be "as if" it was // a wrapping_offset, so we can emulate it as such. This should properly // restore pointer provenance even under today's compiler. - let self_addr = self.addr() as isize; - let dest_addr = addr as isize; - let offset = dest_addr.wrapping_sub(self_addr); - - // This is the canonical desugarring of this operation - self.cast::().wrapping_offset(offset).cast::() + let self_addr = self.addr(); + // Unfortunately, the CHERI-compatible way of defining this operation + // optimizes worse, so we special case it... in a somewhat ad-hoc way + // (checking for 128 bit pointers) because at the time of this writing, + // we don't actually support CHERI yet. Ideally this would be + // `cfg!(target_supports_large_wrapping_offsets)` or something, see + // #96152 for details. + if cfg!(target_pointer_width = "128") { + let offset = (addr as isize).wrapping_sub(self_addr as isize); + // This is the canonical desugarring of this operation + self.cast::().wrapping_offset(offset).cast::() + } else { + self.cast::().wrapping_sub(self_addr).wrapping_add(addr).cast::() + } } /// Creates a new pointer by mapping `self`'s address to a new one. diff --git a/library/core/src/ptr/mut_ptr.rs b/library/core/src/ptr/mut_ptr.rs index 56f9c84f5af6f..827668030bbb8 100644 --- a/library/core/src/ptr/mut_ptr.rs +++ b/library/core/src/ptr/mut_ptr.rs @@ -246,12 +246,20 @@ impl *mut T { // In the mean-time, this operation is defined to be "as if" it was // a wrapping_offset, so we can emulate it as such. This should properly // restore pointer provenance even under today's compiler. - let self_addr = self.addr() as isize; - let dest_addr = addr as isize; - let offset = dest_addr.wrapping_sub(self_addr); - - // This is the canonical desugarring of this operation - self.cast::().wrapping_offset(offset).cast::() + let self_addr = self.addr(); + // Unfortunately, the CHERI-compatible way of defining this operation + // optimizes worse, so we special case it... in a somewhat ad-hoc way + // (checking for 128 bit pointers) because at the time of this writing, + // we don't actually support CHERI yet. Ideally this would be + // `cfg!(target_supports_large_wrapping_offsets)` or something, see + // #96152 for details. + if cfg!(target_pointer_width = "128") { + let offset = (addr as isize).wrapping_sub(self_addr as isize); + // This is the canonical desugarring of this operation + self.cast::().wrapping_offset(offset).cast::() + } else { + self.cast::().wrapping_sub(self_addr).wrapping_add(addr).cast::() + } } /// Creates a new pointer by mapping `self`'s address to a new one. From 35f28d8433022ecadff947c2cadd895767a615c7 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Thu, 28 Apr 2022 19:06:56 -0700 Subject: [PATCH 2/3] Don't bother with the CHERI-compatible version of `ptr::with_addr` --- library/core/src/ptr/const_ptr.rs | 31 ++++++++++++++++++------------- library/core/src/ptr/mut_ptr.rs | 31 ++++++++++++++++++------------- 2 files changed, 36 insertions(+), 26 deletions(-) diff --git a/library/core/src/ptr/const_ptr.rs b/library/core/src/ptr/const_ptr.rs index f0cfd27027dda..787bdea181e71 100644 --- a/library/core/src/ptr/const_ptr.rs +++ b/library/core/src/ptr/const_ptr.rs @@ -243,19 +243,24 @@ impl *const T { // a wrapping_offset, so we can emulate it as such. This should properly // restore pointer provenance even under today's compiler. let self_addr = self.addr(); - // Unfortunately, the CHERI-compatible way of defining this operation - // optimizes worse, so we special case it... in a somewhat ad-hoc way - // (checking for 128 bit pointers) because at the time of this writing, - // we don't actually support CHERI yet. Ideally this would be - // `cfg!(target_supports_large_wrapping_offsets)` or something, see - // #96152 for details. - if cfg!(target_pointer_width = "128") { - let offset = (addr as isize).wrapping_sub(self_addr as isize); - // This is the canonical desugarring of this operation - self.cast::().wrapping_offset(offset).cast::() - } else { - self.cast::().wrapping_sub(self_addr).wrapping_add(addr).cast::() - } + // In an ideal world (err, an ideal world we'd have an intrinsic, but + // short of that), we'd implement this as follows: + // ``` + // let offset = (addr as isize).wrapping_sub(self_addr as isize); + // self.cast::().wrapping_offset(offset).cast::() + // ``` + // This is the canonical desugaring of this operation, and is compatible + // with targets which don't allow large wrapping add/sub/offset + // operations, including CHERI. + // + // Unfortunately, this causes worse codegen than the following + // implementation, which should be correct on all targets we currently + // support (at the moment AFAICT, we don't yet support CHERI, or we'd + // special-case it to use the desugaring listed above). + // + // As a result, we use the following implementation, which would be + // wrong on CHERI, but right everywhere else. + self.cast::().wrapping_sub(self_addr).wrapping_add(addr).cast::() } /// Creates a new pointer by mapping `self`'s address to a new one. diff --git a/library/core/src/ptr/mut_ptr.rs b/library/core/src/ptr/mut_ptr.rs index 827668030bbb8..28e4a7e8e2760 100644 --- a/library/core/src/ptr/mut_ptr.rs +++ b/library/core/src/ptr/mut_ptr.rs @@ -247,19 +247,24 @@ impl *mut T { // a wrapping_offset, so we can emulate it as such. This should properly // restore pointer provenance even under today's compiler. let self_addr = self.addr(); - // Unfortunately, the CHERI-compatible way of defining this operation - // optimizes worse, so we special case it... in a somewhat ad-hoc way - // (checking for 128 bit pointers) because at the time of this writing, - // we don't actually support CHERI yet. Ideally this would be - // `cfg!(target_supports_large_wrapping_offsets)` or something, see - // #96152 for details. - if cfg!(target_pointer_width = "128") { - let offset = (addr as isize).wrapping_sub(self_addr as isize); - // This is the canonical desugarring of this operation - self.cast::().wrapping_offset(offset).cast::() - } else { - self.cast::().wrapping_sub(self_addr).wrapping_add(addr).cast::() - } + // In an ideal world (err, an ideal world we'd have an intrinsic, but + // short of that), we'd implement this as follows: + // ``` + // let offset = (addr as isize).wrapping_sub(self_addr as isize); + // self.cast::().wrapping_offset(offset).cast::() + // ``` + // This is the canonical desugaring of this operation, and is compatible + // with targets which don't allow large wrapping add/sub/offset + // operations, including CHERI. + // + // Unfortunately, this causes worse codegen than the following + // implementation, which should be correct on all targets we currently + // support (at the moment AFAICT, we don't yet support CHERI, or we'd + // special-case it to use the desugaring listed above). + // + // As a result, we use the following implementation, which would be + // wrong on CHERI, but right everywhere else. + self.cast::().wrapping_sub(self_addr).wrapping_add(addr).cast::() } /// Creates a new pointer by mapping `self`'s address to a new one. From 9a902b122ff1dfd346a4c1d538737a75632d3dce Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Thu, 28 Apr 2022 20:04:22 -0700 Subject: [PATCH 3/3] Try swapping wrapping_add and wrapping_sub --- library/core/src/ptr/const_ptr.rs | 2 +- library/core/src/ptr/mut_ptr.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/library/core/src/ptr/const_ptr.rs b/library/core/src/ptr/const_ptr.rs index 787bdea181e71..2c17317b85d1a 100644 --- a/library/core/src/ptr/const_ptr.rs +++ b/library/core/src/ptr/const_ptr.rs @@ -260,7 +260,7 @@ impl *const T { // // As a result, we use the following implementation, which would be // wrong on CHERI, but right everywhere else. - self.cast::().wrapping_sub(self_addr).wrapping_add(addr).cast::() + self.cast::().wrapping_add(addr).wrapping_sub(self_addr).cast::() } /// Creates a new pointer by mapping `self`'s address to a new one. diff --git a/library/core/src/ptr/mut_ptr.rs b/library/core/src/ptr/mut_ptr.rs index 28e4a7e8e2760..30f0c71da98f9 100644 --- a/library/core/src/ptr/mut_ptr.rs +++ b/library/core/src/ptr/mut_ptr.rs @@ -264,7 +264,7 @@ impl *mut T { // // As a result, we use the following implementation, which would be // wrong on CHERI, but right everywhere else. - self.cast::().wrapping_sub(self_addr).wrapping_add(addr).cast::() + self.cast::().wrapping_add(addr).wrapping_sub(self_addr).cast::() } /// Creates a new pointer by mapping `self`'s address to a new one.