Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add API to get slice from CxxVector #321

Closed
wants to merge 2 commits into from
Closed

Add API to get slice from CxxVector #321

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07ac753
Add API to get slice from CxxVector
roligugus Sep 21, 2020
b317a06
Avoid undefined behaviour in slice::from_raw_parts()
roligugus Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions gen/src/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1270,6 +1270,13 @@ fn write_cxx_vector(out: &mut OutFile, vector_ty: &Type, element: &Ident, types:
);
writeln!(out, " return s.size();");
writeln!(out, "}}");
writeln!(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering whether I can or should generate src/cxx.h|cc from this. Need to look into that...

out,
"const {} *cxxbridge04$std$vector${}$data(const ::std::vector<{}> &s) noexcept {{",
inner, instance, inner,
);
writeln!(out, " return s.data();");
writeln!(out, "}}");
writeln!(
out,
"const {} *cxxbridge04$std$vector${}$get_unchecked(const ::std::vector<{}> &s, size_t pos) noexcept {{",
Expand Down
11 changes: 11 additions & 0 deletions macro/src/expand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,7 @@ fn expand_cxx_vector(namespace: &Namespace, elem: &Ident) -> TokenStream {
let name = elem.to_string();
let prefix = format!("cxxbridge04$std$vector${}{}$", namespace, elem);
let link_size = format!("{}size", prefix);
let link_data = format!("{}data", prefix);
let link_get_unchecked = format!("{}get_unchecked", prefix);
let unique_ptr_prefix = format!("cxxbridge04$unique_ptr$std$vector${}{}$", namespace, elem);
let link_unique_ptr_null = format!("{}null", unique_ptr_prefix);
Expand All @@ -859,6 +860,16 @@ fn expand_cxx_vector(namespace: &Namespace, elem: &Ident) -> TokenStream {
}
unsafe { __vector_size(v) }
}
fn __vector_slice(v: &::cxx::CxxVector<Self>) -> &[Self] {
extern "C" {
#[link_name = #link_size]
fn __vector_size(_: &::cxx::CxxVector<#elem>) -> usize;
#[link_name = #link_data]
fn __vector_data(_: &::cxx::CxxVector<#elem>) -> *const #elem;
}
let ret = unsafe { std::slice::from_raw_parts(__vector_data(v), __vector_size(v)) };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is undefined behavior in the case that size is 0. The data ptr of a &[T] has to be nonnull and properly aligned regardless of size, which C++ does not guarantee.

See https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html#safety.

Somewhere there will need to be special handling of the empty vector case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually wondering about that. ;)
Let me add the special casing of nullptr/0 size

Copy link
Author

@roligugus roligugus Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not looked into how rust handles alignment. That could be a biggie if rust has different alignment reqs...

Having said that, we could enforce that in the __vector_data() implementation. As in, if wrongly aligned, "do something". that "do something" needs looking into/discussion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed handling of null data() ptr.

UB only happens if std::vector's capacity() is 0 if I got the from_raw_parts documentation right. There does not seem to be a need to specifically handle 0 size. Famous last words?

Ignore my going off the tangent with non-alignment. I was thinking off T, whereas you were talking about the data ptr if null.

ret
}
unsafe fn __get_unchecked(v: &::cxx::CxxVector<Self>, pos: usize) -> &Self {
extern "C" {
#[link_name = #link_get_unchecked]
Expand Down
4 changes: 4 additions & 0 deletions src/cxx.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ void cxxbridge04$unique_ptr$std$string$drop(
const std::vector<CXX_TYPE> &s) noexcept { \
return s.size(); \
} \
const CXX_TYPE* cxxbridge04$std$vector$##RUST_TYPE##$data( \
const std::vector<CXX_TYPE> &s) noexcept { \
return s.data(); \
} \
const CXX_TYPE *cxxbridge04$std$vector$##RUST_TYPE##$get_unchecked( \
const std::vector<CXX_TYPE> &s, size_t pos) noexcept { \
return &s[pos]; \
Expand Down
26 changes: 26 additions & 0 deletions src/cxx_vector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ use core::fmt::{self, Display};
use core::marker::PhantomData;
use core::mem;
use core::ptr;
use std::slice;


/// Binding to C++ `std::vector<T, std::allocator<T>>`.
///
Expand Down Expand Up @@ -63,6 +65,15 @@ where
pub unsafe fn get_unchecked(&self, pos: usize) -> &T {
T::__get_unchecked(self, pos)
}

/// Returns a slice to the underlying vector data
///
/// An alternative approach would be to base your FFI API directly on a
/// rust::Slice<T> instead of a std::vector<T>.
pub fn get_slice(&self) -> &[T] {
let slice = T::__vector_slice(self);
slice
}
}

pub struct Iter<'a, T> {
Expand Down Expand Up @@ -122,6 +133,7 @@ where
pub unsafe trait VectorElement: Sized {
const __NAME: &'static dyn Display;
fn __vector_size(v: &CxxVector<Self>) -> usize;
fn __vector_slice(v: &CxxVector<Self>) -> &[Self];
unsafe fn __get_unchecked(v: &CxxVector<Self>, pos: usize) -> &Self;
fn __unique_ptr_null() -> *mut c_void;
unsafe fn __unique_ptr_raw(raw: *mut CxxVector<Self>) -> *mut c_void;
Expand All @@ -145,6 +157,20 @@ macro_rules! impl_vector_element {
}
unsafe { __vector_size(v) }
}
fn __vector_slice(v: &CxxVector<$ty>) -> &[$ty] {
extern "C" {
attr! {
#[link_name = concat!("cxxbridge04$std$vector$", $segment, "$data")]
fn __vector_data(_: &CxxVector<$ty>) -> &$ty;
}
attr! {
#[link_name = concat!("cxxbridge04$std$vector$", $segment, "$size")]
fn __vector_size(_: &CxxVector<$ty>) -> usize;
}
}
let ret = unsafe { slice::from_raw_parts(__vector_data(v), __vector_size(v)) };
ret
}
unsafe fn __get_unchecked(v: &CxxVector<$ty>, pos: usize) -> &$ty {
extern "C" {
attr! {
Expand Down
9 changes: 9 additions & 0 deletions tests/ffi/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@ pub mod ffi {
fn r_take_ref_rust_vec_string(v: &Vec<String>);
fn r_take_enum(e: Enum);

fn r_access_vector_u8_as_slice(v: &CxxVector<u8>);

fn r_try_return_void() -> Result<()>;
fn r_try_return_primitive() -> Result<usize>;
fn r_try_return_box() -> Result<Box<R>>;
Expand All @@ -165,6 +167,8 @@ pub type R = usize;

pub struct R2(usize);

use cxx::CxxVector;

impl R2 {
fn get(&self) -> usize {
self.0
Expand Down Expand Up @@ -327,6 +331,11 @@ fn r_take_enum(e: ffi::Enum) {
let _ = e;
}

fn r_access_vector_u8_as_slice(v: &CxxVector<u8>) {
let s = v.get_slice();
assert_eq!(s, [86, 75, 30, 9]);
}

fn r_try_return_void() -> Result<(), Error> {
Ok(())
}
Expand Down
6 changes: 6 additions & 0 deletions tests/ffi/tests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,12 @@ extern "C" const char *cxx_run_test() noexcept {
std::unique_ptr<std::string>(new std::string("2020")));
r_take_enum(Enum::AVal);

// Caller owns data handed into rust APIs
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for that did not really fit into the "take" or "return" tests.

I initially had a c_take...() test that would create a std::vector and then call into the r_access_...() test function, but I found that overly complex as the c_take...() test did not add anything. Hence, directly testing the API in r_access..() here.

{
std::vector<std::uint8_t> v{86, 75, 30, 9};
r_access_vector_u8_as_slice(v);
}

ASSERT(r_try_return_primitive() == 2020);
try {
r_fail_return_primitive();
Expand Down