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

eddyb · 2016-06-26T14:43:19Z

Part of a potential solution for rust-lang/rust#34227.

This change is

Replace return_address usage for rooting with stack guards and convenience macros. The existing `Rooted` and `RootedVec` users were migrated the the following two macros: ```rust let x = Rooted::new(cx, value); // Was changed to: rooted!(in(cx) let x = value); // Which expands to: let mut __root = Rooted::new_unrooted(value); let x = RootedGuard::new(cx, &mut __root); ``` ```rust let mut v = RootedVec::new(); v.extend(iter); // Was changed to: rooted_vec!(let v = iter); // Which expands to: let mut __root = RootableVec::new(); let v = RootedVec::new(&mut __root, iter); ``` The `rooted!` macro depends on servo/rust-mozjs#272. These APIs based on two types, a container to be rooted and a rooting guard, allow implementing both `Rooted`-style rooting and `Traceable`-based rooting in stable Rust, without abusing `return_address`. Such macros may have been tried before, but in 1.9 their hygiene is broken, they work only since 1.10. Sadly, `Rooted` is a FFI type and completely exposed, so I cannot prevent anyone from creating their own, although all fields but the value get overwritten by `RootedGuard::new` anyway. `RootableVec` OTOH is *guaranteed* to be empty when not rooted, which makes it harmless AFAICT. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix rust-lang/rust#34227 - [x] These changes do not require tests because they are not functional changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11872)

Ms2ger · 2016-06-27T09:32:32Z

src/rust.rs

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


Why the Copy bound here and on handle_mut?

The methods to create those types requires it.

Ms2ger · 2016-06-27T09:40:44Z

Others should review this too, but I think this is good enough.

CC @nox @Manishearth @jdm

nox · 2016-06-27T09:41:18Z

LGTM too.

Manishearth · 2016-06-27T09:52:35Z

src/conversions.rs

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

+struct ForOfIteratorGuard<'a> {


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

Manishearth · 2016-06-27T10:00:58Z

Overall lgtm. would like more docs.

Manishearth · 2016-06-27T12:08:03Z

@bors-servo r+

Ms2ger · 2016-06-27T15:24:10Z

@bors-servo r=Manishearth

bors-servo · 2016-06-27T15:24:11Z

📌 Commit 3e1be4a has been approved by Manishearth

Ms2ger · 2016-06-27T15:26:32Z

@bors-servo retry

Ms2ger · 2016-06-27T15:28:47Z

@bors-servo clean retry

Ms2ger · 2016-06-27T15:57:59Z

@bors-servo r=Manishearth

bors-servo · 2016-06-27T15:58:00Z

📌 Commit ae62516 has been approved by Manishearth

bors-servo · 2016-06-27T15:58:03Z

⌛ Testing commit ae62516 with merge 9ddac71...

Replace return_address usage in Rooted with a stack guard and a rooted! macro. Part of a potential solution for rust-lang/rust#34227.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/272)

bors-servo · 2016-06-27T16:21:57Z

💔 Test failed - travis

cbrewster · 2016-06-27T17:39:20Z

@bors-servo r=Manishearth

bors-servo · 2016-06-27T17:39:20Z

📌 Commit 71333b8 has been approved by Manishearth

bors-servo · 2016-06-27T17:39:22Z

⌛ Testing commit 71333b8 with merge 7ccfee5...

Replace return_address usage in Rooted with a stack guard and a rooted! macro. Part of a potential solution for rust-lang/rust#34227.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/272)

bors-servo · 2016-06-27T18:06:38Z

☀️ Test successful - travis

fitzgen · 2016-06-28T19:15:20Z

src/rust.rs

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


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>.

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

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.

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

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

GCMethods::initial

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.

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

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.

Replace return_address usage for rooting with stack guards and convenience macros. The existing `Rooted` and `RootedVec` users were migrated the the following two macros: ```rust let x = Rooted::new(cx, value); // Was changed to: rooted!(in(cx) let x = value); // Which expands to: let mut __root = Rooted::new_unrooted(value); let x = RootedGuard::new(cx, &mut __root); ``` ```rust let mut v = RootedVec::new(); v.extend(iterator); // Was changed to: rooted_vec!(let v <- iterator); // Which expands to: let mut __root = RootableVec::new(); let v = RootedVec::new(&mut __root, iterator); ``` The `rooted!` macro depends on servo/rust-mozjs#272. These APIs based on two types, a container to be rooted and a rooting guard, allow implementing both `Rooted`-style rooting and `Traceable`-based rooting in stable Rust, without abusing `return_address`. Such macros may have been tried before, but in 1.9 their hygiene is broken, they work only since 1.10. Sadly, `Rooted` is a FFI type and completely exposed, so I cannot prevent anyone from creating their own, although all fields but the value get overwritten by `RootedGuard::new` anyway. `RootableVec` OTOH is *guaranteed* to be empty when not rooted, which makes it harmless AFAICT. By fixing rust-lang/rust#34227, this PR enables Servo to build with `-Zorbit`. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix rust-lang/rust#34227 - [x] These changes do not require tests because they are not functional changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11872)

…tack guards and convenience macros (from eddyb:back-to-roots); r=Ms2ger The existing `Rooted` and `RootedVec` users were migrated the the following two macros: ```rust let x = Rooted::new(cx, value); // Was changed to: rooted!(in(cx) let x = value); // Which expands to: let mut __root = Rooted::new_unrooted(value); let x = RootedGuard::new(cx, &mut __root); ``` ```rust let mut v = RootedVec::new(); v.extend(iterator); // Was changed to: rooted_vec!(let v <- iterator); // Which expands to: let mut __root = RootableVec::new(); let v = RootedVec::new(&mut __root, iterator); ``` The `rooted!` macro depends on servo/rust-mozjs#272. These APIs based on two types, a container to be rooted and a rooting guard, allow implementing both `Rooted`-style rooting and `Traceable`-based rooting in stable Rust, without abusing `return_address`. Such macros may have been tried before, but in 1.9 their hygiene is broken, they work only since 1.10. Sadly, `Rooted` is a FFI type and completely exposed, so I cannot prevent anyone from creating their own, although all fields but the value get overwritten by `RootedGuard::new` anyway. `RootableVec` OTOH is *guaranteed* to be empty when not rooted, which makes it harmless AFAICT. By fixing rust-lang/rust#34227, this PR enables Servo to build with `-Zorbit`. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix rust-lang/rust#34227 - [x] These changes do not require tests because they are not functional changes Source-Repo: https://github.com/servo/servo Source-Revision: 80cb0cf8214fd52d2884724614c40cb278ee7575 UltraBlame original commit: 70db9045abfa70e0440944258f92294eb298000c

Remove unused CustomAutoRooter API.

4627586

eddyb added a commit to eddyb/servo that referenced this pull request Jun 26, 2016

Temporarily switch rust-mozjs to servo/rust-mozjs#272.

12250ba

eddyb added a commit to eddyb/servo that referenced this pull request Jun 26, 2016

Temporarily switch rust-mozjs to servo/rust-mozjs#272.

07328e7

eddyb mentioned this pull request Jun 26, 2016

Replace return_address usage for rooting with stack guards and convenience macros. servo/servo#11872

Merged

4 tasks

eddyb added a commit to eddyb/servo that referenced this pull request Jun 26, 2016

Temporarily switch rust-mozjs to servo/rust-mozjs#272.

b1d2f75

eddyb force-pushed the back-to-roots branch 2 times, most recently from 3ad7c45 to 3e1be4a Compare June 26, 2016 17:39

eddyb added a commit to eddyb/servo that referenced this pull request Jun 26, 2016

Temporarily switch rust-mozjs to servo/rust-mozjs#272.

fb3c098

Ms2ger reviewed Jun 27, 2016
View reviewed changes

Manishearth reviewed Jun 27, 2016
View reviewed changes

eddyb force-pushed the back-to-roots branch from 3e1be4a to ae62516 Compare June 27, 2016 12:06

Ms2ger closed this Jun 27, 2016

Ms2ger reopened this Jun 27, 2016

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

71333b8

eddyb force-pushed the back-to-roots branch from ae62516 to 71333b8 Compare June 27, 2016 17:01

bors-servo merged commit 71333b8 into servo:master Jun 27, 2016

eddyb deleted the back-to-roots branch June 27, 2016 18:24

eddyb added a commit to eddyb/servo that referenced this pull request Jun 27, 2016

Update rust-mozjs to include servo/rust-mozjs#272.

61e6cd2

eddyb added a commit to eddyb/servo that referenced this pull request Jun 27, 2016

Update rust-mozjs to include servo/rust-mozjs#272.

6d208ee

eddyb added a commit to eddyb/servo that referenced this pull request Jun 28, 2016

Update rust-mozjs to include servo/rust-mozjs#272.

b79bf8a

fitzgen reviewed Jun 28, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

eddyb commented Jun 26, 2016 •

edited by larsbergstrom

Loading

Ms2ger Jun 27, 2016

eddyb Jun 27, 2016

Ms2ger commented Jun 27, 2016

nox commented Jun 27, 2016

Manishearth Jun 27, 2016

Manishearth commented Jun 27, 2016

Manishearth commented Jun 27, 2016

Ms2ger commented Jun 27, 2016

bors-servo commented Jun 27, 2016

Ms2ger commented Jun 27, 2016

Ms2ger commented Jun 27, 2016

Ms2ger commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bors-servo commented Jun 27, 2016

cbrewster commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bors-servo commented Jun 27, 2016

fitzgen Jun 28, 2016

eddyb Jun 28, 2016

eddyb Jun 28, 2016

Ms2ger Jun 29, 2016

eddyb Jun 29, 2016

nox Jun 29, 2016

eddyb Jun 29, 2016

nox Jun 29, 2016

eddyb Jun 29, 2016

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

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

Conversation

eddyb commented Jun 26, 2016 • edited by larsbergstrom Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ms2ger commented Jun 27, 2016

nox commented Jun 27, 2016

Choose a reason for hiding this comment

Manishearth commented Jun 27, 2016

Manishearth commented Jun 27, 2016

Ms2ger commented Jun 27, 2016

bors-servo commented Jun 27, 2016

Ms2ger commented Jun 27, 2016

Ms2ger commented Jun 27, 2016

Ms2ger commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bors-servo commented Jun 27, 2016

cbrewster commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bors-servo commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Jun 26, 2016 •

edited by larsbergstrom

Loading