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

eddyb · 2016-06-26T15:23:15Z

The existing Rooted and RootedVec users were migrated the the following two macros:

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);

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.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix return_address intrinsic problematic with MIR and hard/impossible to use safely. rust-lang/rust#34227
These changes do not require tests because they are not functional changes

This change is

highfive · 2016-06-26T15:23:17Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/xmlhttprequest.rs, components/script/dom/range.rs, components/script/dom/htmlscriptelement.rs, components/script/devtools.rs, components/script/dom/document.rs, components/script/dom/eventtarget.rs, components/script/timers.rs, components/script/dom/browsingcontext.rs, components/script/dom/bindings/callback.rs, components/script/dom/workerglobalscope.rs, components/script/dom/bindings/error.rs, components/script/lib.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/worker.rs, components/script/webdriver_handlers.rs, components/script/dom/bindings/interface.rs, components/script/dom/macros.rs, components/script/dom/eventdispatcher.rs, components/script/dom/serviceworker.rs, components/script/Cargo.toml, components/script/dom/errorevent.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/bindings/proxyhandler.rs, components/script/dom/bindings/utils.rs, components/script/script_thread.rs, components/script/dom/messageevent.rs, components/script/dom/htmliframeelement.rs, components/script/dom/node.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/bindings/trace.rs, components/script/dom/websocket.rs, components/script/dom/serviceworkerglobalscope.rs
@emilio: components/script/dom/webglrenderingcontext.rs

highfive · 2016-06-26T15:23:18Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2016-06-26T15:29:46Z

@bors-servo: try

bors-servo · 2016-06-26T15:29:49Z

⌛ Trying commit 07328e7 with merge 91b36f4...

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); ``` 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`. 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)

Manishearth · 2016-06-26T15:31:36Z

components/script/dom/macros.rs

+#[macro_export]
+macro_rules! rooted_vec {
+    (let $name:ident = $iter:expr) => {
+        let mut __root = $crate::dom::bindings::trace::RootableVec::new_unrooted();


make these unsafe and make the macro safe?

oh I see what you did there

bors-servo · 2016-06-26T15:33:18Z

💔 Test failed - linux-dev

Manishearth · 2016-06-26T15:33:44Z

r? @jdm

The design for RootedVec seems sound to me (will look at rooted! later), but I'm not as familiar with the intricacies of these bindings as jdm is

Manishearth · 2016-06-26T15:33:56Z

   Compiling glutin_app v0.0.1 (file:///home/servo/buildbot/slave/linux-dev/build/ports/glutin)
/home/servo/buildbot/slave/linux-dev/build/components/script/webdriver_handlers.rs:31:41: 31:55 error: unresolved import `js::jsapi::UndefinedValue`. There is no `UndefinedValue` in `js::jsapi` [E0432]
/home/servo/buildbot/slave/linux-dev/build/components/script/webdriver_handlers.rs:31 use js::jsapi::{JSContext, HandleValue, UndefinedValue};
                                                                                                                              ^~~~~~~~~~~~~~

cbrewster · 2016-06-26T16:43:18Z

@bors-servo try

bors-servo · 2016-06-26T16:43:21Z

⌛ Trying commit b1d2f75 with merge 1b404d5...

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)

bors-servo · 2016-06-26T17:35:05Z

💔 Test failed - linux-rel

eddyb · 2016-06-26T17:38:39Z

I think I have a fix for /_mozilla/mozilla/sequence-hole.html - the Vec<T> FromJSConvertible in jsapi::conversions wasn't unrooting properly.
Is /html/semantics/embedded-content/the-iframe-element/change_parentage.html a legitimate failure?

FWIW, those are the only failures I've seen with -Zorbit on, which is probably good news!

jdm · 2016-06-26T17:56:16Z

change_parentage is #11703.

eddyb · 2016-06-26T18:09:19Z

@jdm The convertible failure is fixed FWIW, I just checked (with -Zorbit no less!). Also, although dromaero is ridiculously variable, switching to MIR seems to universally drop the times down, so that's something (again, if it's not completely random variance). Someone else should look into it.

@pcwalton

Remove the return_address intrinsic. This intrinsic to get the return pointer was introduced in #16248 / #16081 by @pcwalton for Servo. However, as explained in #34227, it's impossible to ensure it's used correctly, and it broke with `-Zorbit`. Servo's usage is being replaced in servo/servo#11872, and I expect nobody else to have abused it. But I've also started a crater run, just in case this is a `[breaking-change]` for anyone else.

bors-servo · 2016-07-04T17:34:39Z

📌 Commit 9584669 has been approved by Ms2ger

bors-servo · 2016-07-04T17:44:41Z

⌛ Testing commit 9584669 with merge 162af13...

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)

bors-servo · 2016-07-04T17:48:05Z

💔 Test failed - linux-dev

jdm · 2016-07-04T18:03:33Z

@bors-servo: r=Ms2ger

bors-servo · 2016-07-04T18:03:34Z

📌 Commit b79a7d4 has been approved by Ms2ger

bors-servo · 2016-07-04T18:03:36Z

⌛ Testing commit b79a7d4 with merge 80cb0cf...

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)

bors-servo · 2016-07-04T18:58:53Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

Revert "Revert "Remove the return_address intrinsic."" This reverts commit f698cd3. Made possible by the merge of servo/servo#11872, this closes #34227 for good.

We don't use it anymore since #11872.

We don't use it anymore since servo#11872.

highfive assigned Manishearth Jun 26, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 26, 2016

eddyb force-pushed the back-to-roots branch from 12250ba to 07328e7 Compare June 26, 2016 15:24

Manishearth reviewed Jun 26, 2016
View reviewed changes

highfive added the S-tests-failed The changes caused existing tests to fail. label Jun 26, 2016

highfive assigned jdm and unassigned Manishearth Jun 26, 2016

eddyb force-pushed the back-to-roots branch from 07328e7 to b1d2f75 Compare June 26, 2016 15:37

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jun 26, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label Jun 26, 2016

eddyb force-pushed the back-to-roots branch from b1d2f75 to fb3c098 Compare June 26, 2016 17:39

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jun 26, 2016

This was referenced Jun 26, 2016

Remove the return_address intrinsic. rust-lang/rust#34491

Merged

Switch to MIR-based translation by default. rust-lang/rust#34096

Merged

highfive assigned Ms2ger and unassigned jdm Jul 4, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 4, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jul 4, 2016

eddyb added 2 commits July 4, 2016 20:59

Switch to using the new rooted!/RootedGuard API for rooting.

0db1faf

Use a stack guard and a macro for RootedVec instead of return_address.

b79a7d4

eddyb force-pushed the back-to-roots branch from 9584669 to b79a7d4 Compare July 4, 2016 18:00

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jul 4, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 4, 2016

bors-servo merged commit b79a7d4 into servo:master Jul 4, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 4, 2016

bors-servo mentioned this pull request Jul 4, 2016

Integrate service worker manager thread #11727

Merged

4 tasks

eddyb deleted the back-to-roots branch July 4, 2016 19:12

eddyb mentioned this pull request Jul 4, 2016

Revert "Revert "Remove the return_address intrinsic."" rust-lang/rust#34648

Closed

nox added a commit that referenced this pull request Aug 13, 2016

Remove rust_tenacious

f12fad7

We don't use it anymore since #11872.

samuknet pushed a commit to samuknet/servo that referenced this pull request Sep 6, 2016

Remove rust_tenacious

d830666

We don't use it anymore since servo#11872.

Uh oh!

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

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

Uh oh!

Conversation

eddyb commented Jun 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jun 26, 2016

Uh oh!

highfive commented Jun 26, 2016

Uh oh!

jdm commented Jun 26, 2016

Uh oh!

bors-servo commented Jun 26, 2016

Uh oh!

Manishearth Jun 26, 2016

Choose a reason for hiding this comment

Uh oh!

Manishearth Jun 26, 2016

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Jun 26, 2016

Uh oh!

Manishearth commented Jun 26, 2016

Uh oh!

Manishearth commented Jun 26, 2016

Uh oh!

cbrewster commented Jun 26, 2016

Uh oh!

bors-servo commented Jun 26, 2016

Uh oh!

bors-servo commented Jun 26, 2016

Uh oh!

eddyb commented Jun 26, 2016

Uh oh!

jdm commented Jun 26, 2016

Uh oh!

eddyb commented Jun 26, 2016

Uh oh!

bors-servo commented Jul 4, 2016

Uh oh!

bors-servo commented Jul 4, 2016

Uh oh!

bors-servo commented Jul 4, 2016

Uh oh!

jdm commented Jul 4, 2016

Uh oh!

bors-servo commented Jul 4, 2016

Uh oh!

bors-servo commented Jul 4, 2016

Uh oh!

bors-servo commented Jul 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

eddyb commented Jun 26, 2016 •

edited

Loading