From 08da921314f7abf154c7b1d3d49df000e3e253dd Mon Sep 17 00:00:00 2001 From: Jakob Buchgraber Date: Tue, 9 Jul 2024 06:59:28 -0700 Subject: [PATCH] Remove the defensive copy on singular string setter Calling into_proxied() already does a copy and before this change we were doing a second one. I am not using set_allocated_, } @@ -87,7 +87,7 @@ pub type RawRepeatedField = NonNull<_opaque_pointees::RawRepeatedFieldData>; pub type RawMap = NonNull<_opaque_pointees::RawMapData>; /// A raw pointer to a std::string. -type CppStdString = NonNull<_opaque_pointees::CppStdStringData>; +pub type CppStdString = NonNull<_opaque_pointees::CppStdStringData>; /// Kernel-specific owned `string` and `bytes` field type. #[derive(Debug)] @@ -109,6 +109,11 @@ impl InnerProtoString { // SAFETY: `self.owned_ptr` points to a valid std::string object. unsafe { proto2_rust_cpp_string_to_view(self.owned_ptr).as_ref() } } + + pub fn into_raw(self, _private: Private) -> CppStdString { + let s = ManuallyDrop::new(self); + s.owned_ptr + } } impl From<&[u8]> for InnerProtoString { @@ -219,7 +224,7 @@ impl SerializedData { pub fn into_vec(self) -> Vec { // We need to prevent self from being dropped, because we are going to transfer // ownership of self.data to the Vec. - let s = std::mem::ManuallyDrop::new(self); + let s = ManuallyDrop::new(self); unsafe { // SAFETY: @@ -352,14 +357,6 @@ impl<'msg> MutatorMessageRef<'msg> { } } -pub fn copy_bytes_in_arena_if_needed_by_runtime<'msg>( - _msg_ref: MutatorMessageRef<'msg>, - val: &'msg [u8], -) -> &'msg [u8] { - // Nothing to do, the message manages its own string memory for C++. - val -} - /// The raw type-erased version of an owned `Repeated`. #[derive(Debug)] pub struct InnerRepeated { @@ -430,8 +427,7 @@ impl CppTypeConversions for ProtoString { } fn into_insertelem(v: Self) -> CppStdString { - let v = ManuallyDrop::new(v); - v.inner.owned_ptr + v.into_inner(Private).into_raw(Private) } } @@ -444,8 +440,7 @@ impl CppTypeConversions for ProtoBytes { } fn into_insertelem(v: Self) -> CppStdString { - let v = ManuallyDrop::new(v); - v.inner.owned_ptr + v.into_inner(Private).into_raw(Private) } } @@ -817,13 +812,11 @@ fn ptrlen_to_str<'msg>(val: PtrAndLen) -> &'msg ProtoStr { } fn protostr_into_cppstdstring(val: ProtoString) -> CppStdString { - let m = ManuallyDrop::new(val); - m.inner.owned_ptr + val.into_inner(Private).into_raw(Private) } fn protobytes_into_cppstdstring(val: ProtoBytes) -> CppStdString { - let m = ManuallyDrop::new(val); - m.inner.owned_ptr + val.into_inner(Private).into_raw(Private) } // Warning: this function is unsound on its own! `val.as_ref()` must be safe to diff --git a/rust/string.rs b/rust/string.rs index 3f82e14b67..a94f4efc00 100644 --- a/rust/string.rs +++ b/rust/string.rs @@ -29,6 +29,15 @@ pub struct ProtoBytes { pub(crate) inner: InnerProtoString, } +impl ProtoBytes { + // Returns the kernel-specific container. This method is private in spirit and + // must not be called by a user. + #[doc(hidden)] + pub fn into_inner(self, _private: Private) -> InnerProtoString { + self.inner + } +} + impl AsRef<[u8]> for ProtoBytes { fn as_ref(&self) -> &[u8] { self.inner.as_bytes() @@ -162,6 +171,13 @@ impl ProtoString { pub fn as_bytes(&self) -> &[u8] { self.inner.as_bytes() } + + // Returns the kernel-specific container. This method is private in spirit and + // must not be called by a user. + #[doc(hidden)] + pub fn into_inner(self, _private: Private) -> InnerProtoString { + self.inner + } } impl From for ProtoBytes { diff --git a/rust/upb.rs b/rust/upb.rs index e53e0aa98f..5a8cdfac87 100644 --- a/rust/upb.rs +++ b/rust/upb.rs @@ -128,13 +128,6 @@ impl<'msg> MutatorMessageRef<'msg> { } } -pub fn copy_bytes_in_arena_if_needed_by_runtime<'msg>( - msg_ref: MutatorMessageRef<'msg>, - val: &'msg [u8], -) -> &'msg [u8] { - copy_bytes_in_arena(msg_ref.arena, val) -} - fn copy_bytes_in_arena<'msg>(arena: &'msg Arena, val: &'msg [u8]) -> &'msg [u8] { // SAFETY: the alignment of `[u8]` is less than `UPB_MALLOC_ALIGN`. let new_alloc = unsafe { arena.alloc(Layout::for_value(val)) }; @@ -157,6 +150,12 @@ impl InnerProtoString { pub(crate) fn as_bytes(&self) -> &[u8] { &self.0 } + + #[doc(hidden)] + pub fn into_raw_parts(self, _private: Private) -> (PtrAndLen, Arena) { + let (data_ptr, arena) = self.0.into_parts(); + (unsafe { data_ptr.as_ref().into() }, arena) + } } impl From<&[u8]> for InnerProtoString { @@ -617,12 +616,11 @@ impl UpbTypeConversions for ProtoBytes { ) -> upb_MessageValue { // SAFETY: The arena memory is not freed due to `ManuallyDrop`. let parent_arena = ManuallyDrop::new(unsafe { Arena::from_raw(raw_parent_arena) }); - let (data, arena) = val.inner.0.into_parts(); + let (view, arena) = val.inner.into_raw_parts(Private); parent_arena.fuse(&arena); - let msg_val = Self::to_message_value(unsafe { data.as_ref() }); - msg_val + upb_MessageValue { str_val: view } } unsafe fn from_message_value<'msg>(msg: upb_MessageValue) -> View<'msg, ProtoBytes> { diff --git a/src/google/protobuf/compiler/rust/accessors/singular_string.cc b/src/google/protobuf/compiler/rust/accessors/singular_string.cc index d37bb75dc9..99f8b9ff56 100644 --- a/src/google/protobuf/compiler/rust/accessors/singular_string.cc +++ b/src/google/protobuf/compiler/rust/accessors/singular_string.cc @@ -66,28 +66,46 @@ void SingularString::InMsgImpl(Context& ctx, const FieldDescriptor& field, } )rs"); }}, - {"setter", + {"setter_impl", [&] { - if (accessor_case == AccessorCase::VIEW) return; - ctx.Emit({{"as_ref_method", - (field.type() == FieldDescriptor::TYPE_STRING - ? "as_bytes()" - : "as_ref()")}}, - R"rs( - pub fn set_$raw_field_name$(&mut self, val: impl $pb$::IntoProxied<$proxied_type$>) { - let into_proxied = val.into_proxied($pbi$::Private); - let string_view: $pbr$::PtrAndLen = - $pbr$::copy_bytes_in_arena_if_needed_by_runtime( - self.as_mutator_message_ref($pbi$::Private), - into_proxied.$as_ref_method$, - ).into(); + if (ctx.is_cpp()) { + ctx.Emit(R"rs( + let s = val.into_proxied($pbi$::Private); + unsafe { + $setter_thunk$( + self.as_mutator_message_ref($pbi$::Private).msg(), + s.into_inner($pbi$::Private).into_raw($pbi$::Private) + ); + } + )rs"); + } else { + ctx.Emit(R"rs( + let s = val.into_proxied($pbi$::Private); + let (view, arena) = + s.into_inner($pbi$::Private).into_raw_parts($pbi$::Private); + + let mm_ref = + self.as_mutator_message_ref($pbi$::Private); + let parent_arena = mm_ref.arena($pbi$::Private); + + parent_arena.fuse(&arena); unsafe { $setter_thunk$( self.as_mutator_message_ref($pbi$::Private).msg(), - string_view + view ); } + )rs"); + } + }}, + {"setter", + [&] { + if (accessor_case == AccessorCase::VIEW) return; + ctx.Emit({}, + R"rs( + pub fn set_$raw_field_name$(&mut self, val: impl $pb$::IntoProxied<$proxied_type$>) { + $setter_impl$ } )rs"); }}, @@ -123,6 +141,18 @@ void SingularString::InExternC(Context& ctx, ctx.Emit({{"hazzer_thunk", ThunkName(ctx, field, "has")}, {"getter_thunk", ThunkName(ctx, field, "get")}, {"setter_thunk", ThunkName(ctx, field, "set")}, + {"setter", + [&] { + if (ctx.is_cpp()) { + ctx.Emit(R"rs( + fn $setter_thunk$(raw_msg: $pbr$::RawMessage, val: $pbr$::CppStdString); + )rs"); + } else { + ctx.Emit(R"rs( + fn $setter_thunk$(raw_msg: $pbr$::RawMessage, val: $pbr$::PtrAndLen); + )rs"); + } + }}, {"clearer_thunk", ThunkName(ctx, field, "clear")}, {"with_presence_fields_thunks", [&] { @@ -136,7 +166,7 @@ void SingularString::InExternC(Context& ctx, R"rs( $with_presence_fields_thunks$ fn $getter_thunk$(raw_msg: $pbr$::RawMessage) -> $pbr$::PtrAndLen; - fn $setter_thunk$(raw_msg: $pbr$::RawMessage, val: $pbr$::PtrAndLen); + $setter$ )rs"); } @@ -165,8 +195,9 @@ void SingularString::InThunkCc(Context& ctx, absl::string_view val = msg->$field$(); return ::google::protobuf::rust::PtrAndLen(val.data(), val.size()); } - void $setter_thunk$($QualifiedMsg$* msg, ::google::protobuf::rust::PtrAndLen s) { - msg->set_$field$(absl::string_view(s.ptr, s.len)); + void $setter_thunk$($QualifiedMsg$* msg, std::string* s) { + msg->set_$field$(std::move(*s)); + delete s; } )cc"); }