From f92edc13c210c8e530639330d0de17e63b114bdf Mon Sep 17 00:00:00 2001 From: Marcel Hlopko Date: Tue, 6 Jun 2023 03:14:29 -0700 Subject: [PATCH] Automated rollback of commit fe7c4f94223ad2e6190102fbe3473ce3be19ec48. PiperOrigin-RevId: 538128679 --- .github/workflows/test_rust.yml | 3 +- rust/BUILD | 62 +++++- rust/cpp_kernel/BUILD | 6 - rust/cpp_kernel/cpp.rs | 144 ++++++------- rust/protobuf.rs | 4 +- rust/{common.rs => shared.rs} | 14 +- rust/test/cpp/interop/BUILD | 2 +- rust/test/cpp/interop/main.rs | 9 +- .../rust_proto_library_unit_test.bzl | 4 +- rust/upb_kernel/BUILD | 10 +- rust/upb_kernel/upb.rs | 201 +++++++++--------- .../protobuf/compiler/rust/generator.cc | 2 +- 12 files changed, 242 insertions(+), 219 deletions(-) rename rust/{common.rs => shared.rs} (88%) diff --git a/.github/workflows/test_rust.yml b/.github/workflows/test_rust.yml index c971ce3b96..29eef50ca5 100644 --- a/.github/workflows/test_rust.yml +++ b/.github/workflows/test_rust.yml @@ -27,6 +27,7 @@ jobs: credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} bazel-cache: rust_linux bazel: | - test //rust/upb_kernel:upb_test //rust/cpp_kernel:cpp_test \ + test //rust:protobuf_upb_test //rust:protobuf_cpp_test \ + //rust/upb_kernel:upb_test //rust/cpp_kernel:cpp_test \ //rust/test/rust_proto_library_unit_test:rust_upb_aspect_test \ //rust/upb_kernel:upb_test //src/google/protobuf/compiler/rust/... \ No newline at end of file diff --git a/rust/BUILD b/rust/BUILD index af69c573dd..e38f5644a2 100644 --- a/rust/BUILD +++ b/rust/BUILD @@ -1,6 +1,6 @@ # Protobuf Rust runtime packages. -load("@rules_rust//rust:defs.bzl", "rust_library") +load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") load("@bazel_skylib//rules:common_settings.bzl", "string_flag") load("@rules_proto//proto:defs.bzl", "proto_lang_toolchain") @@ -30,22 +30,68 @@ rust_library( "//conditions:default": ["--cfg=cpp_kernel"], }), deps = select({ - ":use_upb_kernel": ["//rust/upb_kernel:upb"], - "//conditions:default": ["//rust/cpp_kernel:cpp"], + ":use_upb_kernel": [":protobuf_upb"], + "//conditions:default": [":protobuf_cpp"], }), ) +# Represents Rust Protobuf runtime using the upb kernel. +# +# `rust_upb_proto_library` implicitly depends on this target. This target cannot depend on +# `:rust_proto_library_kernel` build setting; it has to be fully functional under any value of that +# setting. +# +# `shared.rs` contains kernel-agnostic logic and simple kernel-specific logic controlled by +# `#[cfg(...)]` attributes. That forces us to compile this file twice, once for each kernel. As a +# result this file is declared in both `:protobuf_upb` and `:protobuf_cpp`. This is in principle +# identical to how we compile regular Rust source files twice (once for production, and once for +# unittesting). rust_library( - name = "common", - srcs = ["common.rs"], - visibility = ["//rust:__subpackages__"], + name = "protobuf_upb", + srcs = ["shared.rs"], + rustc_flags = ["--cfg=upb_kernel"], + deps = ["//rust/upb_kernel:upb"], +) + +rust_test( + name = "protobuf_upb_test", + crate = ":protobuf_upb", + rustc_flags = ["--cfg=upb_kernel"], + tags = [ + # TODO(b/270274576): Enable testing on arm once we have a Rust Arm toolchain. + "not_build:arm", + ], +) + +# Represents Rust Protobuf runtime using the cpp kernel. +# +# `rust_cpp_proto_library` implicitly depends on this target. This target cannot depend on +# `:rust_proto_library_kernel` build setting; it has to be fully functional under any value of that +# setting. +# +# See the comment for `:protobuf` for discussion of `shared.rs` file. +rust_library( + name = "protobuf_cpp", + srcs = ["shared.rs"], + rustc_flags = ["--cfg=cpp_kernel"], + deps = ["//rust/cpp_kernel:cpp"], +) + +rust_test( + name = "protobuf_cpp_test", + crate = ":protobuf_cpp", + rustc_flags = ["--cfg=cpp_kernel"], + tags = [ + # TODO(b/270274576): Enable testing on arm once we have a Rust Arm toolchain. + "not_build:arm", + ], ) proto_lang_toolchain( name = "proto_rust_upb_toolchain", command_line = "--rust_out=experimental-codegen=enabled,kernel=upb:$(OUT)", progress_message = "Generating Rust proto_library %{label}", - runtime = "//rust/upb_kernel:upb", + runtime = ":protobuf_upb", visibility = ["//visibility:public"], ) @@ -53,7 +99,7 @@ proto_lang_toolchain( name = "proto_rust_cpp_toolchain", command_line = "--rust_out=experimental-codegen=enabled,kernel=cpp:$(OUT)", progress_message = "Generating Rust proto_library %{label}", - runtime = "//rust/cpp_kernel:cpp", + runtime = ":protobuf_cpp", visibility = ["//visibility:public"], ) diff --git a/rust/cpp_kernel/BUILD b/rust/cpp_kernel/BUILD index 98b59eb98c..3750498db5 100644 --- a/rust/cpp_kernel/BUILD +++ b/rust/cpp_kernel/BUILD @@ -2,11 +2,6 @@ load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") -# Represents Rust Protobuf runtime using the cpp kernel. -# -# `rust_cpp_proto_library` implicitly depends on this target. This target cannot depend on -# `:rust_proto_library_kernel` build setting; it has to be fully functional under any value of that -# setting. rust_library( name = "cpp", srcs = ["cpp.rs"], @@ -14,7 +9,6 @@ rust_library( "//src/google/protobuf:__subpackages__", "//rust:__subpackages__", ], - deps = ["//rust:common"], ) rust_test( diff --git a/rust/cpp_kernel/cpp.rs b/rust/cpp_kernel/cpp.rs index bcda75690b..64e202b74e 100644 --- a/rust/cpp_kernel/cpp.rs +++ b/rust/cpp_kernel/cpp.rs @@ -30,24 +30,79 @@ // Rust Protobuf runtime using the C++ kernel. -pub use common::ParseError; -pub use common::PtrAndLen; -use std::fmt; -use std::ops::Deref; -use std::ptr::NonNull; -use std::slice; - use std::alloc; use std::alloc::Layout; +use std::boxed::Box; use std::cell::UnsafeCell; +use std::fmt; use std::marker::PhantomData; use std::mem::MaybeUninit; +use std::ops::Deref; +use std::ptr::NonNull; +use std::slice; + +/// A wrapper over a `proto2::Arena`. +/// +/// This is not a safe wrapper per se, because the allocation functions still +/// have sharp edges (see their safety docs for more info). +/// +/// This is an owning type and will automatically free the arena when +/// dropped. +/// +/// Note that this type is neither `Sync` nor `Send`. +pub struct Arena { + ptr: NonNull, + _not_sync: PhantomData>, +} + +impl Arena { + /// Allocates a fresh arena. + #[inline] + pub fn new() -> Self { + Self { ptr: NonNull::dangling(), _not_sync: PhantomData } + } + + /// Returns the raw, C++-managed pointer to the arena. + #[inline] + pub fn raw(&self) -> ! { + unimplemented!() + } + + /// Allocates some memory on the arena. + /// + /// # Safety + /// + /// `layout`'s alignment must be less than `UPB_MALLOC_ALIGN`. + #[inline] + pub unsafe fn alloc(&self, layout: Layout) -> &mut [MaybeUninit] { + unimplemented!() + } + + /// Resizes some memory on the arena. + /// + /// # Safety + /// + /// After calling this function, `ptr` is essentially zapped. `old` must + /// be the layout `ptr` was allocated with via [`Arena::alloc()`]. `new`'s + /// alignment must be less than `UPB_MALLOC_ALIGN`. + #[inline] + pub unsafe fn resize(&self, ptr: *mut u8, old: Layout, new: Layout) -> &[MaybeUninit] { + unimplemented!() + } +} -/// Represents serialized Protobuf wire format data. It's typically produced -/// by `.serialize()`. +impl Drop for Arena { + #[inline] + fn drop(&mut self) { + // unimplemented + } +} + +/// Represents serialized Protobuf wire format data. It's typically produced by +/// `.serialize()`. /// -/// This struct is ABI compatible with the equivalent struct on the C++ -/// side. It owns (and drops) its data. +/// This struct is ABI compatible with the equivalent struct on the C++ side. It +/// owns (and drops) its data. // copybara:strip_begin // LINT.IfChange // copybara:strip_end @@ -88,76 +143,9 @@ impl fmt::Debug for SerializedData { } } -pub mod __runtime { - use super::*; - - /// A wrapper over a `proto2::Arena`. - /// - /// This is not a safe wrapper per se, because the allocation functions - /// still have sharp edges (see their safety docs for more info). - /// - /// This is an owning type and will automatically free the arena when - /// dropped. - /// - /// Note that this type is neither `Sync` nor `Send`. - pub struct Arena { - _ptr: NonNull, - _not_sync: PhantomData>, - } - - impl Arena { - /// Allocates a fresh arena. - #[inline] - pub fn new() -> Self { - Self { _ptr: NonNull::dangling(), _not_sync: PhantomData } - } - - /// Returns the raw, C++-managed pointer to the arena. - #[inline] - pub fn raw(&self) -> ! { - unimplemented!() - } - - /// Allocates some memory on the arena. - /// - /// # Safety - /// - /// `layout`'s alignment must be less than `UPB_MALLOC_ALIGN`. - #[inline] - pub unsafe fn alloc(&self, _layout: Layout) -> &mut [MaybeUninit] { - unimplemented!() - } - - /// Resizes some memory on the arena. - /// - /// # Safety - /// - /// After calling this function, `ptr` is essentially zapped. `old` must - /// be the layout `ptr` was allocated with via [`Arena::alloc()`]. - /// `new`'s alignment must be less than `UPB_MALLOC_ALIGN`. - #[inline] - pub unsafe fn resize( - &self, - _ptr: *mut u8, - _old: Layout, - _new: Layout, - ) -> &[MaybeUninit] { - unimplemented!() - } - } - - impl Drop for Arena { - #[inline] - fn drop(&mut self) { - // unimplemented - } - } -} // mod __runtime - #[cfg(test)] mod tests { use super::*; - use std::boxed::Box; // We need to allocate the byte array so SerializedData can own it and // deallocate it in its drop. This function makes it easier to do so for our diff --git a/rust/protobuf.rs b/rust/protobuf.rs index 657cee4ef3..3569b1c6ab 100644 --- a/rust/protobuf.rs +++ b/rust/protobuf.rs @@ -36,6 +36,6 @@ //! cpp and upb kernels from user code. #[cfg(cpp_kernel)] -pub use cpp::*; +pub use protobuf_cpp::*; #[cfg(upb_kernel)] -pub use upb::*; +pub use protobuf_upb::*; diff --git a/rust/common.rs b/rust/shared.rs similarity index 88% rename from rust/common.rs rename to rust/shared.rs index d6f3215471..b576ca6929 100644 --- a/rust/common.rs +++ b/rust/shared.rs @@ -28,7 +28,17 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -//! Kernel-agnostic logic for Rust Protobuf Runtimes. +//! Kernel-agnostic logic for the Rust Protobuf Runtime. +//! +//! For kernel-specific logic this crate delegates to the respective __runtime +//! crate. + +#[cfg(cpp_kernel)] +pub extern crate cpp as __runtime; +#[cfg(upb_kernel)] +pub extern crate upb as __runtime; + +pub use __runtime::SerializedData; use std::fmt; use std::slice; @@ -57,4 +67,4 @@ impl PtrAndLen { pub unsafe fn as_ref<'a>(self) -> &'a [u8] { slice::from_raw_parts(self.ptr, self.len) } -} +} \ No newline at end of file diff --git a/rust/test/cpp/interop/BUILD b/rust/test/cpp/interop/BUILD index 93b7f85a6d..ac08b426b6 100644 --- a/rust/test/cpp/interop/BUILD +++ b/rust/test/cpp/interop/BUILD @@ -21,7 +21,7 @@ rust_test( ], deps = [ ":test_utils", - "//rust/cpp_kernel:cpp", + "//rust:protobuf_cpp", "//rust/test:unittest_cc_rust_proto", ], ) diff --git a/rust/test/cpp/interop/main.rs b/rust/test/cpp/interop/main.rs index acc7b4fa85..ef84d99512 100644 --- a/rust/test/cpp/interop/main.rs +++ b/rust/test/cpp/interop/main.rs @@ -48,10 +48,10 @@ macro_rules! proto_assert_eq { extern "C" { fn DeserializeTestAllTypes(data: *const u8, len: usize) -> NonNull; fn MutateTestAllTypes(msg: NonNull); - fn SerializeTestAllTypes(msg: NonNull) -> cpp::SerializedData; + fn SerializeTestAllTypes(msg: NonNull) -> protobuf_cpp::SerializedData; fn NewWithExtension() -> NonNull; - fn GetBytesExtension(msg: NonNull) -> cpp::PtrAndLen; + fn GetBytesExtension(msg: NonNull) -> protobuf_cpp::PtrAndLen; } #[test] @@ -110,7 +110,8 @@ fn smuggle_extension() { let mut msg2 = TestAllExtensions::new(); msg2.deserialize(&data).unwrap(); - let bytes = - unsafe { GetBytesExtension(msg2.__unstable_cpp_repr_grant_permission_to_break()).as_ref() }; + let bytes = unsafe { + GetBytesExtension(msg2.__unstable_cpp_repr_grant_permission_to_break()).as_ref() + }; assert_eq!(&*bytes, b"smuggled"); } diff --git a/rust/test/rust_proto_library_unit_test/rust_proto_library_unit_test.bzl b/rust/test/rust_proto_library_unit_test/rust_proto_library_unit_test.bzl index 84c3c96a17..3faf76b3d7 100644 --- a/rust/test/rust_proto_library_unit_test/rust_proto_library_unit_test.bzl +++ b/rust/test/rust_proto_library_unit_test/rust_proto_library_unit_test.bzl @@ -57,7 +57,7 @@ def _rust_upb_aspect_test_impl(ctx): rustc_action = _find_action_with_mnemonic(actions, "Rustc") # The action needs to have the Rust runtime as an input - _find_rust_lib_input(rustc_action.inputs, "upb") + _find_rust_lib_input(rustc_action.inputs, "protobuf") # The action needs to produce a .rlib artifact (sometimes .rmeta as well, not tested here). asserts.true(env, rustc_action.outputs.to_list()[0].path.endswith(".rlib")) @@ -88,7 +88,7 @@ def _rust_cc_aspect_test_impl(ctx): rustc_action = _find_action_with_mnemonic(actions, "Rustc") # The action needs to have the Rust runtime as an input - _find_rust_lib_input(rustc_action.inputs, "cpp") + _find_rust_lib_input(rustc_action.inputs, "protobuf") # The action needs to produce a .rlib artifact (sometimes .rmeta as well, not tested here). asserts.true(env, rustc_action.outputs.to_list()[0].path.endswith(".rlib")) diff --git a/rust/upb_kernel/BUILD b/rust/upb_kernel/BUILD index 345804ed14..233c26024f 100644 --- a/rust/upb_kernel/BUILD +++ b/rust/upb_kernel/BUILD @@ -2,11 +2,6 @@ load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") -# Represents Rust Protobuf runtime using the upb kernel. -# -# `rust_upb_proto_library` implicitly depends on this target. This target cannot depend on -# `:rust_proto_library_kernel` build setting; it has to be fully functional under any value of that -# setting. rust_library( name = "upb", srcs = ["upb.rs"], @@ -14,10 +9,7 @@ rust_library( "//src/google/protobuf:__subpackages__", "//rust:__subpackages__", ], - deps = [ - ":upb_c_api", - "//rust:common", - ], + deps = [":upb_c_api"], ) rust_test( diff --git a/rust/upb_kernel/upb.rs b/rust/upb_kernel/upb.rs index 0e74283593..f19e4dd154 100644 --- a/rust/upb_kernel/upb.rs +++ b/rust/upb_kernel/upb.rs @@ -30,18 +30,105 @@ //! UPB FFI wrapper code for use by Rust Protobuf. -pub use common::ParseError; -pub use common::PtrAndLen; -use std::fmt; -use std::ops::Deref; -use std::ptr::NonNull; -use std::slice; - use std::alloc; use std::alloc::Layout; use std::cell::UnsafeCell; +use std::fmt; use std::marker::PhantomData; use std::mem::MaybeUninit; +use std::ops::Deref; +use std::ptr::NonNull; +use std::slice; + +/// See `upb/port/def.inc`. +const UPB_MALLOC_ALIGN: usize = 8; + +/// A UPB-managed pointer to a raw arena. +pub type RawArena = NonNull; + +/// The data behind a [`RawArena`]. Do not use this type. +#[repr(C)] +pub struct RawArenaData { + _data: [u8; 0], +} + +/// A wrapper over a `upb_Arena`. +/// +/// This is not a safe wrapper per se, because the allocation functions still +/// have sharp edges (see their safety docs for more info). +/// +/// This is an owning type and will automatically free the arena when +/// dropped. +/// +/// Note that this type is neither `Sync` nor `Send`. +pub struct Arena { + raw: RawArena, + _not_sync: PhantomData>, +} + +extern "C" { + fn upb_Arena_New() -> RawArena; + fn upb_Arena_Free(arena: RawArena); + fn upb_Arena_Malloc(arena: RawArena, size: usize) -> *mut u8; + fn upb_Arena_Realloc(arena: RawArena, ptr: *mut u8, old: usize, new: usize) -> *mut u8; +} + +impl Arena { + /// Allocates a fresh arena. + #[inline] + pub fn new() -> Self { + Self { raw: unsafe { upb_Arena_New() }, _not_sync: PhantomData } + } + + /// Returns the raw, UPB-managed pointer to the arena. + #[inline] + pub fn raw(&self) -> RawArena { + self.raw + } + + /// Allocates some memory on the arena. + /// + /// # Safety + /// + /// `layout`'s alignment must be less than `UPB_MALLOC_ALIGN`. + #[inline] + pub unsafe fn alloc(&self, layout: Layout) -> &mut [MaybeUninit] { + debug_assert!(layout.align() <= UPB_MALLOC_ALIGN); + let ptr = upb_Arena_Malloc(self.raw, layout.size()); + if ptr.is_null() { + alloc::handle_alloc_error(layout); + } + + slice::from_raw_parts_mut(ptr.cast(), layout.size()) + } + + /// Resizes some memory on the arena. + /// + /// # Safety + /// + /// After calling this function, `ptr` is essentially zapped. `old` must + /// be the layout `ptr` was allocated with via [`Arena::alloc()`]. `new`'s + /// alignment must be less than `UPB_MALLOC_ALIGN`. + #[inline] + pub unsafe fn resize(&self, ptr: *mut u8, old: Layout, new: Layout) -> &[MaybeUninit] { + debug_assert!(new.align() <= UPB_MALLOC_ALIGN); + let ptr = upb_Arena_Realloc(self.raw, ptr, old.size(), new.size()); + if ptr.is_null() { + alloc::handle_alloc_error(new); + } + + slice::from_raw_parts_mut(ptr.cast(), new.size()) + } +} + +impl Drop for Arena { + #[inline] + fn drop(&mut self) { + unsafe { + upb_Arena_Free(self.raw); + } + } +} /// Represents serialized Protobuf wire format data. /// @@ -51,11 +138,11 @@ pub struct SerializedData { len: usize, // The arena that owns `data`. - _arena: __runtime::Arena, + _arena: Arena, } impl SerializedData { - pub unsafe fn from_raw_parts(arena: __runtime::Arena, data: NonNull, len: usize) -> Self { + pub unsafe fn from_raw_parts(arena: Arena, data: NonNull, len: usize) -> Self { SerializedData { _arena: arena, data, len } } } @@ -73,104 +160,8 @@ impl fmt::Debug for SerializedData { } } -pub mod __runtime { - - use super::*; - - /// See `upb/port/def.inc`. - const UPB_MALLOC_ALIGN: usize = 8; - - /// A UPB-managed pointer to a raw arena. - pub type RawArena = NonNull; - - /// The data behind a [`RawArena`]. Do not use this type. - #[repr(C)] - pub struct RawArenaData { - _data: [u8; 0], - } - - /// A wrapper over a `upb_Arena`. - /// - /// This is not a safe wrapper per se, because the allocation functions - /// still have sharp edges (see their safety docs for more info). - /// - /// This is an owning type and will automatically free the arena when - /// dropped. - /// - /// Note that this type is neither `Sync` nor `Send`. - pub struct Arena { - raw: RawArena, - _not_sync: PhantomData>, - } - - extern "C" { - fn upb_Arena_New() -> RawArena; - fn upb_Arena_Free(arena: RawArena); - fn upb_Arena_Malloc(arena: RawArena, size: usize) -> *mut u8; - fn upb_Arena_Realloc(arena: RawArena, ptr: *mut u8, old: usize, new: usize) -> *mut u8; - } - - impl Arena { - /// Allocates a fresh arena. - #[inline] - pub fn new() -> Self { - Self { raw: unsafe { upb_Arena_New() }, _not_sync: PhantomData } - } - - /// Returns the raw, UPB-managed pointer to the arena. - #[inline] - pub fn raw(&self) -> RawArena { - self.raw - } - - /// Allocates some memory on the arena. - /// - /// # Safety - /// - /// `layout`'s alignment must be less than `UPB_MALLOC_ALIGN`. - #[inline] - pub unsafe fn alloc(&self, layout: Layout) -> &mut [MaybeUninit] { - debug_assert!(layout.align() <= UPB_MALLOC_ALIGN); - let ptr = upb_Arena_Malloc(self.raw, layout.size()); - if ptr.is_null() { - alloc::handle_alloc_error(layout); - } - - slice::from_raw_parts_mut(ptr.cast(), layout.size()) - } - - /// Resizes some memory on the arena. - /// - /// # Safety - /// - /// After calling this function, `ptr` is essentially zapped. `old` must - /// be the layout `ptr` was allocated with via [`Arena::alloc()`]. - /// `new`'s alignment must be less than `UPB_MALLOC_ALIGN`. - #[inline] - pub unsafe fn resize(&self, ptr: *mut u8, old: Layout, new: Layout) -> &[MaybeUninit] { - debug_assert!(new.align() <= UPB_MALLOC_ALIGN); - let ptr = upb_Arena_Realloc(self.raw, ptr, old.size(), new.size()); - if ptr.is_null() { - alloc::handle_alloc_error(new); - } - - slice::from_raw_parts_mut(ptr.cast(), new.size()) - } - } - - impl Drop for Arena { - #[inline] - fn drop(&mut self) { - unsafe { - upb_Arena_Free(self.raw); - } - } - } -} // mod __runtime - #[cfg(test)] mod tests { - use super::__runtime::*; use super::*; #[test] diff --git a/src/google/protobuf/compiler/rust/generator.cc b/src/google/protobuf/compiler/rust/generator.cc index 0de62ee6b9..6ec0e6d3d8 100644 --- a/src/google/protobuf/compiler/rust/generator.cc +++ b/src/google/protobuf/compiler/rust/generator.cc @@ -102,7 +102,7 @@ bool RustGenerator::Generate(const FileDescriptor* file_desc, }); file.Emit({{"kernel", KernelRsName(file.opts().kernel)}}, R"rs( - extern crate $kernel$ as __pb; + extern crate protobuf_$kernel$ as __pb; extern crate std as __std; )rs");