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.

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

Replace return_address usage in Rooted with a stack guard and a rooted! macro. #272

Merged
merged 2 commits into from Jun 27, 2016
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Prev

Move from using return_address to a macro-mediated rooting solution.

  • Loading branch information
eddyb committed Jun 27, 2016
commit 71333b8f824bd8adb6198c070ec033722106dc92
@@ -33,7 +33,7 @@ use jsapi::JSPROP_ENUMERATE;
use jsapi::{JSContext, JSObject, JSString, HandleValue, MutableHandleValue};
use jsapi::{JS_NewUCStringCopyN, JS_StringHasLatin1Chars, JS_WrapValue};
use jsapi::{JS_GetLatin1StringCharsAndLength, JS_GetTwoByteStringCharsAndLength};
use jsapi::{JS_NewArrayObject1, JS_DefineElement, RootedValue, RootedObject};
use jsapi::{JS_NewArrayObject1, JS_DefineElement, RootedObject};
use jsapi::{ForOfIterator, ForOfIterator_NonIterableBehavior};
use jsval::{BooleanValue, Int32Value, NullValue, UInt32Value, UndefinedValue};
use jsval::{JSVal, ObjectValue, ObjectOrNullValue, StringValue};
@@ -473,10 +473,10 @@ impl<T: FromJSValConvertible> FromJSValConvertible for Option<T> {
// https://heycam.github.io/webidl/#es-sequence
impl<T: ToJSValConvertible> ToJSValConvertible for Vec<T> {
unsafe fn to_jsval(&self, cx: *mut JSContext, rval: MutableHandleValue) {
let js_array = RootedObject::new(cx, JS_NewArrayObject1(cx, self.len() as libc::size_t));
rooted!(in(cx) let js_array = JS_NewArrayObject1(cx, self.len() as libc::size_t));
assert!(!js_array.handle().is_null());

let mut val = RootedValue::new(cx, UndefinedValue());
rooted!(in(cx) let mut val = UndefinedValue());
for (index, obj) in self.iter().enumerate() {
obj.to_jsval(cx, val.handle_mut());

@@ -488,6 +488,33 @@ impl<T: ToJSValConvertible> ToJSValConvertible for Vec<T> {
}
}

/// Rooting guard for the iterator field of ForOfIterator.
/// Behaves like RootedGuard (roots on creation, unroots on drop),
/// but borrows and allows access to the whole ForOfIterator, so
/// that methods on ForOfIterator can still be used through it.
struct ForOfIteratorGuard<'a> {

This comment has been minimized.

@Manishearth

Manishearth Jun 27, 2016

Member

documentation on this? (explaining why it is necessary perhaps)

root: &'a mut ForOfIterator
}

impl<'a> ForOfIteratorGuard<'a> {
fn new(cx: *mut JSContext, root: &'a mut ForOfIterator) -> Self {
unsafe {
root.iterator.add_to_root_stack(cx);
}
ForOfIteratorGuard {
root: root
}
}
}

impl<'a> Drop for ForOfIteratorGuard<'a> {
fn drop(&mut self) {
unsafe {
self.root.iterator.remove_from_root_stack();
}
}
}

impl<C: Clone, T: FromJSValConvertible<Config=C>> FromJSValConvertible for Vec<T> {
type Config = C;

@@ -497,9 +524,11 @@ impl<C: Clone, T: FromJSValConvertible<Config=C>> FromJSValConvertible for Vec<T
-> Result<Vec<T>, ()> {
let mut iterator = ForOfIterator {
cx_: cx,
iterator: RootedObject::new(cx, ptr::null_mut()),
iterator: RootedObject::new_unrooted(ptr::null_mut()),
index: ::std::u32::MAX, // NOT_ARRAY
};
let mut iterator = ForOfIteratorGuard::new(cx, &mut iterator);
let iterator = &mut *iterator.root;

if !iterator.init(value, ForOfIterator_NonIterableBehavior::ThrowOnNonIterable) {
return Err(())
@@ -509,7 +538,7 @@ impl<C: Clone, T: FromJSValConvertible<Config=C>> FromJSValConvertible for Vec<T

loop {
let mut done = false;
let mut val = RootedValue::new(cx, UndefinedValue());
rooted!(in(cx) let mut val = UndefinedValue());
if !iterator.next(val.handle_mut(), &mut done) {
return Err(())
}
@@ -5,7 +5,6 @@
#![crate_name = "js"]
#![crate_type = "rlib"]

#![feature(core_intrinsics)]
#![feature(filling_drop)]
#![feature(link_args)]
#![feature(unsafe_no_drop_flag)]
@@ -78,12 +77,15 @@ pub mod jsapi {
include!("jsapi_linux_32_debug.rs");
}

#[macro_use]
pub mod rust;

mod consts;
pub mod conversions;
pub mod error;
pub mod glue;
pub mod jsval;
pub mod rust;


pub use consts::*;

@@ -13,7 +13,6 @@ use std::slice;
use std::mem;
use std::u32;
use std::default::Default;
use std::intrinsics::return_address;
use std::ops::{Deref, DerefMut};
use std::cell::UnsafeCell;
use std::marker::PhantomData;
@@ -269,39 +268,101 @@ impl RootKind for Value {
fn rootKind() -> jsapi::RootKind { jsapi::RootKind::Value }
}

impl<T: RootKind + Copy> Rooted<T> {
pub fn new_with_addr(cx: *mut JSContext, initial: T, addr: *const u8) -> Rooted<T> {
let ctxfriend: &mut ContextFriendFields = unsafe {
mem::transmute(cx)
};

let kind = T::rootKind() as usize;
let root = Rooted::<T> {
impl<T> Rooted<T> {
pub fn new_unrooted(initial: T) -> Rooted<T> {

This comment has been minimized.

@fitzgen

fitzgen Jun 28, 2016

Member

My gut reaction to seeing an unrooted "Rooted<T>" is terror: it means you could pass around references to Rooted<T> (what would be [Mutable]Handle<T> in js/) that are not in fact rooted.

Is there any way we could use the type system to differentiate between things that are actually rooted vs those that are not (hashtag session types?), or maybe even just rename these types? It seems to me that Rooted<T> is really Rootable<T> and RootedGuard<T> is the real Rooted<T>.

This comment has been minimized.

@eddyb

eddyb Jun 28, 2016

Author Contributor

You cannot create a Handle safely from it. A reference to Rooted means nothing by itself.

This comment has been minimized.

@eddyb

eddyb Jun 28, 2016

Author Contributor

I would love to use better names but Rooted is an FFI type (which is required because it's a node in a linked list) so we cannot do too much about it.

This comment has been minimized.

@Ms2ger

Ms2ger Jun 29, 2016

Collaborator

Can we move the initial assignment to RootedGuard::new()?

This comment has been minimized.

@eddyb

eddyb Jun 29, 2016

Author Contributor

I suppose so? But what do you put there in Rooted::new_unrooted?

This comment has been minimized.

This comment has been minimized.

@eddyb

eddyb Jun 29, 2016

Author Contributor

That trait seems like a good choice to have as a bound in the struct itself.
Alright, other changes suggested on IRC were to have struct Rootable<T>(Rooted<T>); which could only be created empty and not accessed, and it's what RootedGuard would accept.

This comment has been minimized.

@nox

nox Jun 29, 2016

Member

You can do the wrapper that cannot be accessed and fill it with initial.

This comment has been minimized.

@eddyb

eddyb Jun 29, 2016

Author Contributor

However, that wrapper would cause issues with ForOfIterator, although that's internal to rust-mozjs and perhaps could be accommodated for with a pub(crate) unsafe method.

Rooted {
_base: RootedBase { _phantom0: PhantomData },
stack: &mut ctxfriend.roots.stackRoots_[kind] as *mut _ as *mut _,
prev: ctxfriend.roots.stackRoots_[kind] as *mut _,
stack: ptr::null_mut(),
prev: ptr::null_mut(),
ptr: initial,
};
}
}

ctxfriend.roots.stackRoots_[kind] = unsafe { mem::transmute(addr) };
root
pub unsafe fn add_to_root_stack(&mut self, cx: *mut JSContext) where T: RootKind {
let ctxfriend: &mut ContextFriendFields = mem::transmute(cx);

let kind = T::rootKind() as usize;
self.stack = &mut ctxfriend.roots.stackRoots_[kind] as *mut _ as *mut _;
self.prev = ctxfriend.roots.stackRoots_[kind] as *mut _;

ctxfriend.roots.stackRoots_[kind] = self as *mut _ as usize as _;
}

pub fn new(cx: *mut JSContext, initial: T) -> Rooted<T> {
Rooted::new_with_addr(cx, initial, unsafe { return_address() })
pub unsafe fn remove_from_root_stack(&mut self) {
assert!(*self.stack == mem::transmute(&*self));
*self.stack = self.prev;
}
}

pub fn handle(&self) -> Handle<T> {
/// Rust API for keeping a Rooted value in the context's root stack.
/// Example usage: `rooted!(in(cx) let x = UndefinedValue());`.
/// `RootedGuard::new` also works, but the macro is preferred.
pub struct RootedGuard<'a, T: 'a> {
root: &'a mut Rooted<T>

This comment has been minimized.

@fitzgen

fitzgen Jun 28, 2016

Member

The idea is that the Rooted<T> can safely be added to the root stack because it cannot be moved by the compiler while there is an active &mut reference to it?

This comment has been minimized.

@eddyb

eddyb Jun 28, 2016

Author Contributor

Yes, borrowed lvalues cannot be moved from in Rust. That's a core guarantee.

}

impl<'a, T> RootedGuard<'a, T> {
pub fn new(cx: *mut JSContext, root: &'a mut Rooted<T>) -> Self where T: RootKind {
unsafe {
root.add_to_root_stack(cx);
}
RootedGuard {
root: root
}
}

pub fn handle(&self) -> Handle<T> where T: Copy {

This comment has been minimized.

@Ms2ger

Ms2ger Jun 27, 2016

Collaborator

Why the Copy bound here and on handle_mut?

This comment has been minimized.

@eddyb

eddyb Jun 27, 2016

Author Contributor

The methods to create those types requires it.

unsafe {
Handle::from_marked_location(&self.ptr)
Handle::from_marked_location(&self.root.ptr)
}
}

pub fn handle_mut(&mut self) -> MutableHandle<T> {
pub fn handle_mut(&mut self) -> MutableHandle<T> where T: Copy {
unsafe {
MutableHandle::from_marked_location(&mut self.ptr)
MutableHandle::from_marked_location(&mut self.root.ptr)
}
}

pub fn get(&self) -> T where T: Copy {
self.root.ptr
}

pub fn set(&mut self, v: T) {
self.root.ptr = v;
}
}

impl<'a, T> Deref for RootedGuard<'a, T> {
type Target = T;
fn deref(&self) -> &T {
&self.root.ptr
}
}

impl<'a, T> DerefMut for RootedGuard<'a, T> {
fn deref_mut(&mut self) -> &mut T {
&mut self.root.ptr
}
}

impl<'a, T> Drop for RootedGuard<'a, T> {
fn drop(&mut self) {
unsafe {
self.root.remove_from_root_stack();
}
}
}

#[macro_export]
macro_rules! rooted {
(in($cx:expr) let $name:ident = $init:expr) => {
let mut __root = $crate::jsapi::Rooted::new_unrooted($init);
let $name = $crate::rust::RootedGuard::new($cx, &mut __root);
};
(in($cx:expr) let mut $name:ident = $init:expr) => {
let mut __root = $crate::jsapi::Rooted::new_unrooted($init);
let mut $name = $crate::rust::RootedGuard::new($cx, &mut __root);
}
}

impl<T: Copy> Handle<T> {
@@ -390,18 +451,6 @@ impl<T: Copy> MutableHandle<T> {
}
}

impl<T> Drop for Rooted<T> {
fn drop(&mut self) {
unsafe {
if self.stack as usize == mem::POST_DROP_USIZE {
return;
}
assert!(*self.stack == mem::transmute(&*self));
*self.stack = self.prev;
}
}
}

impl Default for jsid {
fn default() -> jsid { JSID_VOID }
}
@@ -2,6 +2,7 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#[macro_use]
extern crate js;
extern crate libc;

@@ -14,8 +15,6 @@ use js::jsapi::JS_EncodeStringToUTF8;
use js::jsapi::JS_NewGlobalObject;
use js::jsapi::JS_ReportError;
use js::jsapi::OnNewGlobalHookOption;
use js::jsapi::Rooted;
use js::jsapi::RootedValue;
use js::jsapi::Value;
use js::jsval::UndefinedValue;
use js::rust::{Runtime, SIMPLE_GLOBAL_CLASS};
@@ -33,14 +32,14 @@ fn callback() {

unsafe {
let global = JS_NewGlobalObject(context, &SIMPLE_GLOBAL_CLASS, ptr::null_mut(), h_option, &c_option);
let global_root = Rooted::new(context, global);
rooted!(in(context) let global_root = global);
let global = global_root.handle();
let _ac = JSAutoCompartment::new(context, global.get());
let function = JS_DefineFunction(context, global, b"puts\0".as_ptr() as *const libc::c_char,
Some(puts), 1, 0);
assert!(!function.is_null());
let javascript = "puts('Test Iñtërnâtiônàlizætiøn ┬─┬ノ( º _ ºノ) ');";
let mut rval = RootedValue::new(runtime.cx(), UndefinedValue());
rooted!(in(context) let mut rval = UndefinedValue());
let _ = runtime.evaluate_script(global, javascript, "test.js", 0, rval.handle_mut());
}
}
@@ -55,7 +54,7 @@ unsafe extern "C" fn puts(context: *mut JSContext, argc: u32, vp: *mut Value) ->

let arg = args.get(0);
let js = js::rust::ToString(context, arg);
let message_root = Rooted::new(context, js);
rooted!(in(context) let message_root = js);
let message = JS_EncodeStringToUTF8(context, message_root.handle());
let message = CStr::from_ptr(message);
println!("{}", str::from_utf8(message.to_bytes()).unwrap());
@@ -2,13 +2,12 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

#[macro_use]
extern crate js;

use js::jsapi::CompartmentOptions;
use js::jsapi::JS_NewGlobalObject;
use js::jsapi::OnNewGlobalHookOption;
use js::jsapi::RootedObject;
use js::jsapi::RootedValue;
use js::jsval::UndefinedValue;
use js::rust::{Runtime, SIMPLE_GLOBAL_CLASS};

@@ -21,12 +20,12 @@ fn evaluate() {

unsafe {

let global = RootedObject::new(cx,
rooted!(in(cx) let global =
JS_NewGlobalObject(cx, &SIMPLE_GLOBAL_CLASS, ptr::null_mut(),
OnNewGlobalHookOption::FireOnNewGlobalHook,
&CompartmentOptions::default())
);
let mut rval = RootedValue::new(cx, UndefinedValue());
rooted!(in(cx) let mut rval = UndefinedValue());
assert!(rt.evaluate_script(global.handle(), "1 + 1",
"test", 1, rval.handle_mut()).is_ok());
}
@@ -2,13 +2,12 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

#[macro_use]
extern crate js;

use js::jsapi::CompartmentOptions;
use js::jsapi::JS_NewGlobalObject;
use js::jsapi::OnNewGlobalHookOption;
use js::jsapi::Rooted;
use js::jsapi::RootedValue;
use js::jsval::UndefinedValue;
use js::rust::{Runtime, SIMPLE_GLOBAL_CLASS};

@@ -24,9 +23,9 @@ fn stack_limit() {
let c_option = CompartmentOptions::default();
let global = JS_NewGlobalObject(cx, &SIMPLE_GLOBAL_CLASS,
ptr::null_mut(), h_option, &c_option);
let global_root = Rooted::new(cx, global);
rooted!(in(cx) let global_root = global);
let global = global_root.handle();
let mut rval = RootedValue::new(cx, UndefinedValue());
rooted!(in(cx) let mut rval = UndefinedValue());
assert!(rt.evaluate_script(global, "function f() { f.apply() } f()",
"test", 1, rval.handle_mut()).is_err());
}
@@ -2,6 +2,7 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

#[macro_use]
extern crate js;

use js::conversions::ConversionBehavior;
@@ -12,8 +13,6 @@ use js::jsapi::JSAutoCompartment;
use js::jsapi::JS_InitStandardClasses;
use js::jsapi::JS_NewGlobalObject;
use js::jsapi::OnNewGlobalHookOption;
use js::jsapi::Rooted;
use js::jsapi::RootedValue;
use js::jsval::UndefinedValue;
use js::rust::{Runtime, SIMPLE_GLOBAL_CLASS};

@@ -30,13 +29,13 @@ fn vec_conversion() {
unsafe {
let global = JS_NewGlobalObject(cx, &SIMPLE_GLOBAL_CLASS,
ptr::null_mut(), h_option, &c_option);
let global_root = Rooted::new(cx, global);
rooted!(in(cx) let global_root = global);
let global = global_root.handle();

let _ac = JSAutoCompartment::new(cx, global.get());
assert!(JS_InitStandardClasses(cx, global));

let mut rval = RootedValue::new(cx, UndefinedValue());
rooted!(in(cx) let mut rval = UndefinedValue());

let orig_vec: Vec<f32> = vec![1.0, 2.9, 3.0];
orig_vec.to_jsval(cx, rval.handle_mut());
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.