Remove several unsafes from the FFI bridge code. Change-Id: I554b0bef5869032a70997e2c5af1a37f6a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/24540 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/src/gn/ffi/bridge.cc b/src/gn/ffi/bridge.cc index df8eb27..63b4b22 100644 --- a/src/gn/ffi/bridge.cc +++ b/src/gn/ffi/bridge.cc
@@ -592,6 +592,7 @@ struct Any; struct SliceAny; struct KeyValue; +struct ParseNodePtr; enum class ValueType : ::std::uint8_t; using Err = ::Err; using InputFile = ::InputFile; @@ -633,6 +634,15 @@ }; #endif // CXXBRIDGE1_STRUCT_KeyValue +#ifndef CXXBRIDGE1_STRUCT_ParseNodePtr +#define CXXBRIDGE1_STRUCT_ParseNodePtr +struct ParseNodePtr final { + ::ParseNode const *ptr CXX_DEFAULT_VALUE(nullptr); + + using IsRelocatable = ::std::true_type; +}; +#endif // CXXBRIDGE1_STRUCT_ParseNodePtr + #ifndef CXXBRIDGE1_ENUM_ValueType #define CXXBRIDGE1_ENUM_ValueType enum class ValueType : ::std::uint8_t { @@ -776,34 +786,34 @@ return (self.*scope_value$)(); } -void cxxbridge1$196$SetValueNone(::Value &val, ::ParseNode const *origin) noexcept { - void (*SetValueNone$)(::Value &, ::ParseNode const *) = ::SetValueNone; - SetValueNone$(val, origin); +void cxxbridge1$196$SetValueNone(::Value &val, ::ParseNodePtr *origin) noexcept { + void (*SetValueNone$)(::Value &, ::ParseNodePtr) = ::SetValueNone; + SetValueNone$(val, ::std::move(*origin)); } -void cxxbridge1$196$SetValueBool(::Value &val, ::ParseNode const *origin, bool b) noexcept { - void (*SetValueBool$)(::Value &, ::ParseNode const *, bool) = ::SetValueBool; - SetValueBool$(val, origin, b); +void cxxbridge1$196$SetValueBool(::Value &val, ::ParseNodePtr *origin, bool b) noexcept { + void (*SetValueBool$)(::Value &, ::ParseNodePtr, bool) = ::SetValueBool; + SetValueBool$(val, ::std::move(*origin), b); } -void cxxbridge1$196$SetValueInt(::Value &val, ::ParseNode const *origin, ::std::int64_t i) noexcept { - void (*SetValueInt$)(::Value &, ::ParseNode const *, ::std::int64_t) = ::SetValueInt; - SetValueInt$(val, origin, i); +void cxxbridge1$196$SetValueInt(::Value &val, ::ParseNodePtr *origin, ::std::int64_t i) noexcept { + void (*SetValueInt$)(::Value &, ::ParseNodePtr, ::std::int64_t) = ::SetValueInt; + SetValueInt$(val, ::std::move(*origin), i); } -void cxxbridge1$196$SetValueString(::Value &val, ::ParseNode const *origin, ::rust::Str s) noexcept { - void (*SetValueString$)(::Value &, ::ParseNode const *, ::rust::Str) = ::SetValueString; - SetValueString$(val, origin, s); +void cxxbridge1$196$SetValueString(::Value &val, ::ParseNodePtr *origin, ::rust::Str s) noexcept { + void (*SetValueString$)(::Value &, ::ParseNodePtr, ::rust::Str) = ::SetValueString; + SetValueString$(val, ::std::move(*origin), s); } -::Any *cxxbridge1$196$SetValueList(::Value &val, ::ParseNode const *origin, ::std::size_t size) noexcept { - ::Any *(*SetValueList$)(::Value &, ::ParseNode const *, ::std::size_t) = ::SetValueList; - return SetValueList$(val, origin, size); +::Any *cxxbridge1$196$SetValueList(::Value &val, ::ParseNodePtr *origin, ::std::size_t size) noexcept { + ::Any *(*SetValueList$)(::Value &, ::ParseNodePtr, ::std::size_t) = ::SetValueList; + return SetValueList$(val, ::std::move(*origin), size); } -void cxxbridge1$196$SetValueScope(::Value &val, ::ParseNode const *origin, ::Scope *scope) noexcept { - void (*SetValueScope$)(::Value &, ::ParseNode const *, ::std::unique_ptr<::Scope>) = ::SetValueScope; - SetValueScope$(val, origin, ::std::unique_ptr<::Scope>(scope)); +void cxxbridge1$196$SetValueScope(::Value &val, ::ParseNodePtr *origin, ::Scope *scope) noexcept { + void (*SetValueScope$)(::Value &, ::ParseNodePtr, ::std::unique_ptr<::Scope>) = ::SetValueScope; + SetValueScope$(val, ::std::move(*origin), ::std::unique_ptr<::Scope>(scope)); } static_assert(::rust::detail::is_complete<::std::remove_extent<::Err>::type>::value, "definition of `::Err` is required");
diff --git a/src/gn/ffi/bridge.h b/src/gn/ffi/bridge.h index fe645f9..e4baff8 100644 --- a/src/gn/ffi/bridge.h +++ b/src/gn/ffi/bridge.h
@@ -575,6 +575,7 @@ struct Any; struct SliceAny; struct KeyValue; +struct ParseNodePtr; enum class ValueType : ::std::uint8_t; using Err = ::Err; using InputFile = ::InputFile; @@ -616,6 +617,15 @@ }; #endif // CXXBRIDGE1_STRUCT_KeyValue +#ifndef CXXBRIDGE1_STRUCT_ParseNodePtr +#define CXXBRIDGE1_STRUCT_ParseNodePtr +struct ParseNodePtr final { + ::ParseNode const *ptr CXX_DEFAULT_VALUE(nullptr); + + using IsRelocatable = ::std::true_type; +}; +#endif // CXXBRIDGE1_STRUCT_ParseNodePtr + #ifndef CXXBRIDGE1_ENUM_ValueType #define CXXBRIDGE1_ENUM_ValueType enum class ValueType : ::std::uint8_t {
diff --git a/src/gn/ffi/value.cc b/src/gn/ffi/value.cc index dce39d8..4904326 100644 --- a/src/gn/ffi/value.cc +++ b/src/gn/ffi/value.cc
@@ -20,32 +20,32 @@ return sizeof(Value); } -void SetValueNone(Value& self, const ParseNode* origin) { - new (&self) Value(origin, Value::NONE); +void SetValueNone(Value& self, ParseNodePtr origin) { + new (&self) Value(origin.ptr, Value::NONE); } -void SetValueBool(Value& self, const ParseNode* origin, bool b) { - new (&self) Value(origin, b); +void SetValueBool(Value& self, ParseNodePtr origin, bool b) { + new (&self) Value(origin.ptr, b); } -void SetValueInt(Value& self, const ParseNode* origin, int64_t i) { - new (&self) Value(origin, i); +void SetValueInt(Value& self, ParseNodePtr origin, int64_t i) { + new (&self) Value(origin.ptr, i); } -void SetValueString(Value& self, const ParseNode* origin, rust::Str s) { - new (&self) Value(origin, std::string(s.data(), s.size())); +void SetValueString(Value& self, ParseNodePtr origin, rust::Str s) { + new (&self) Value(origin.ptr, std::string(s.data(), s.size())); } -Any* SetValueList(Value& self, const ParseNode* origin, size_t size) { - new (&self) Value(origin, Value::LIST); +Any* SetValueList(Value& self, ParseNodePtr origin, size_t size) { + new (&self) Value(origin.ptr, Value::LIST); self.list_value().resize(size); return reinterpret_cast<Any*>(self.list_value().data()); } void SetValueScope(Value& self, - const ParseNode* origin, + ParseNodePtr origin, std::unique_ptr<Scope> scope) { - new (&self) Value(origin, std::move(scope)); + new (&self) Value(origin.ptr, std::move(scope)); } SliceAny GetValueList(const Value& self) {
diff --git a/src/gn/ffi/value.h b/src/gn/ffi/value.h index 5fe8106..16e2853 100644 --- a/src/gn/ffi/value.h +++ b/src/gn/ffi/value.h
@@ -11,7 +11,7 @@ #include "gn/value.h" class Scope; -class ParseNode; +struct ParseNodePtr; struct SliceAny; enum class ValueType : uint8_t; @@ -25,18 +25,18 @@ // SetValue* is called with potentially uninitialized Value objects. // These functions roughly correspond to calling the corresponding constructor // with in-place construction. -void SetValueNone(Value& self, const ParseNode* origin); -void SetValueBool(Value& self, const ParseNode* origin, bool b); -void SetValueInt(Value& self, const ParseNode* origin, int64_t i); -void SetValueString(Value& self, const ParseNode* origin, rust::Str s); +void SetValueNone(Value& self, ParseNodePtr origin); +void SetValueBool(Value& self, ParseNodePtr origin, bool b); +void SetValueInt(Value& self, ParseNodePtr origin, int64_t i); +void SetValueString(Value& self, ParseNodePtr origin, rust::Str s); struct Any; // Sets the value to a list of `size` elements. Returns a pointer to the start // of the vector. // // Safety: Rust is required to convert this to a Slice<Value>(pointer, size) -Any* SetValueList(Value& self, const ParseNode* origin, size_t size); +Any* SetValueList(Value& self, ParseNodePtr origin, size_t size); void SetValueScope(Value& self, - const ParseNode* origin, + ParseNodePtr origin, std::unique_ptr<Scope> scope); // Returns a "std::vector<Value>". //
diff --git a/src/gn/starlark/crates/ffi/src/bridge.rs b/src/gn/starlark/crates/ffi/src/bridge.rs index ee7c45f..28ae838 100644 --- a/src/gn/starlark/crates/ffi/src/bridge.rs +++ b/src/gn/starlark/crates/ffi/src/bridge.rs
@@ -32,6 +32,13 @@ value: &'a Value, } + // cxxbridge marks any function that takes a raw pointer unsafe. + // By providing a thin wrapper around the pointer, we can remove the unsafe. + #[derive(Clone, Copy, Default)] + struct ParseNodePtr { + ptr: *const ParseNode, + } + #[derive(Clone, Copy)] enum ValueType { None = 0, @@ -147,32 +154,20 @@ #[cxx_name = "GetValueList"] pub(in crate::value) fn list_value_cxx(val: &Value) -> SliceAny; pub(in crate::value) fn scope_value(self: &Value) -> *const Scope; - pub(in crate::value) unsafe fn SetValueNone(val: Pin<&mut Value>, origin: *const ParseNode); - pub(in crate::value) unsafe fn SetValueBool( - val: Pin<&mut Value>, - origin: *const ParseNode, - b: bool, - ); - pub(in crate::value) unsafe fn SetValueInt( - val: Pin<&mut Value>, - origin: *const ParseNode, - i: i64, - ); - pub(in crate::value) unsafe fn SetValueString( - val: Pin<&mut Value>, - origin: *const ParseNode, - s: &str, - ); + pub(in crate::value) fn SetValueNone(val: Pin<&mut Value>, origin: ParseNodePtr); + pub(in crate::value) fn SetValueBool(val: Pin<&mut Value>, origin: ParseNodePtr, b: bool); + pub(in crate::value) fn SetValueInt(val: Pin<&mut Value>, origin: ParseNodePtr, i: i64); + pub(in crate::value) fn SetValueString(val: Pin<&mut Value>, origin: ParseNodePtr, s: &str); // Initialises self as a list of `size` elements and returns a pointer to the // start. - pub(in crate::value) unsafe fn SetValueList( + pub(in crate::value) fn SetValueList( val: Pin<&mut Value>, - origin: *const ParseNode, + origin: ParseNodePtr, size: usize, ) -> *mut Any; - pub(in crate::value) unsafe fn SetValueScope( + pub(in crate::value) fn SetValueScope( val: Pin<&mut Value>, - origin: *const ParseNode, + origin: ParseNodePtr, scope: UniquePtr<Scope>, ); }
diff --git a/src/gn/starlark/crates/ffi/src/scope.rs b/src/gn/starlark/crates/ffi/src/scope.rs index 54a43a7..532346e 100644 --- a/src/gn/starlark/crates/ffi/src/scope.rs +++ b/src/gn/starlark/crates/ffi/src/scope.rs
@@ -54,7 +54,7 @@ for (placeholder, val) in placeholders.as_slice_mut().iter_mut().zip(vals) { placeholder .as_mut() - .assign(val, child_ref, std::ptr::null()); + .assign(val, child_ref, Default::default()); } Self(child_scope)
diff --git a/src/gn/starlark/crates/ffi/src/value.rs b/src/gn/starlark/crates/ffi/src/value.rs index 39bd977..dd970e4 100644 --- a/src/gn/starlark/crates/ffi/src/value.rs +++ b/src/gn/starlark/crates/ffi/src/value.rs
@@ -6,7 +6,6 @@ use starlark::values::{list::ListRef, structs::StructRef}; -pub use crate::bridge::ParseNode; use crate::{ bridge::{SliceAny, Value, ValueType}, Immutable, Scope, Slice, @@ -43,32 +42,19 @@ mut self: Pin<&mut Self>, val: starlark::values::Value<'v>, scope: &mut Scope, - origin: *const ParseNode, + origin: crate::bridge::ParseNodePtr, ) { if val.is_none() { - // Safety: Just an FFI function. - unsafe { - crate::bridge::SetValueNone(self.as_mut(), origin); - } + crate::bridge::SetValueNone(self.as_mut(), origin); } else if let Some(s) = val.unpack_str() { - // Safety: Just an FFI function. - unsafe { - crate::bridge::SetValueString(self.as_mut(), origin, s); - } + crate::bridge::SetValueString(self.as_mut(), origin, s); } else if let Some(b) = val.unpack_bool() { - // Safety: Just an FFI function. - unsafe { - crate::bridge::SetValueBool(self.as_mut(), origin, b); - } + crate::bridge::SetValueBool(self.as_mut(), origin, b); } else if let Some(i) = val.unpack_i32() { - // Safety: Just an FFI function. - unsafe { - crate::bridge::SetValueInt(self.as_mut(), origin, i64::from(i)); - } + crate::bridge::SetValueInt(self.as_mut(), origin, i64::from(i)); } else if let Some(l) = ListRef::from_value(val) { let mut slice: Slice<Self> = SliceAny { - // Safety: Just an FFI function. - ptr: unsafe { crate::bridge::SetValueList(self.as_mut(), origin, l.len()) }, + ptr: crate::bridge::SetValueList(self.as_mut(), origin, l.len()), len: l.len(), } .into(); @@ -83,10 +69,7 @@ v_cxx.as_mut().assign(v_starlark, scope, origin); } - // Safety: Just an FFI function. - unsafe { - crate::bridge::SetValueScope(self.as_mut(), origin, nested_scope); - } + crate::bridge::SetValueScope(self.as_mut(), origin, nested_scope); } else { todo!("Arbitrary starlark values not (yet) supported"); } @@ -108,7 +91,13 @@ let scope = setup.scope(); let mut value = crate::bridge::NewValueForTesting(); - value.pin_mut().assign(val, scope, std::ptr::null()); + value.pin_mut().assign( + val, + scope, + crate::bridge::ParseNodePtr { + ptr: std::ptr::null(), + }, + ); value.to_rust(heap) }