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 for rooting with stack guards and convenience macros. #11872

Merged
merged 2 commits into from Jul 4, 2016

Conversation

@eddyb
Copy link
Contributor

eddyb commented Jun 26, 2016

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 rust-lang/rust#34227
  • These changes do not require tests because they are not functional changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Jun 26, 2016

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

This comment has been minimized.

Copy link

highfive commented Jun 26, 2016

warning Warning 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!
@eddyb eddyb force-pushed the eddyb:back-to-roots branch from 12250ba to 07328e7 Jun 26, 2016
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jun 26, 2016

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jun 26, 2016

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

bors-servo added a commit that referenced this pull request Jun 26, 2016
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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11872)
<!-- Reviewable:end -->
#[macro_export]
macro_rules! rooted_vec {
(let $name:ident = $iter:expr) => {
let mut __root = $crate::dom::bindings::trace::RootableVec::new_unrooted();

This comment has been minimized.

Copy link
@Manishearth

Manishearth Jun 26, 2016

Member

make these unsafe and make the macro safe?

This comment has been minimized.

Copy link
@Manishearth

Manishearth Jun 26, 2016

Member

oh I see what you did there

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jun 26, 2016

💔 Test failed - linux-dev

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Jun 26, 2016

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

@highfive highfive assigned jdm and unassigned Manishearth Jun 26, 2016
@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Jun 26, 2016

   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};
                                                                                                                              ^~~~~~~~~~~~~~
@eddyb eddyb force-pushed the eddyb:back-to-roots branch from 07328e7 to b1d2f75 Jun 26, 2016
@cbrewster

This comment has been minimized.

Copy link
Member

cbrewster commented Jun 26, 2016

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jun 26, 2016

⌛️ Trying commit b1d2f75 with merge 1b404d5...

bors-servo added a commit that referenced this pull request Jun 26, 2016
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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11872)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jun 26, 2016

💔 Test failed - linux-rel

@eddyb

This comment has been minimized.

Copy link
Contributor Author

eddyb commented Jun 26, 2016

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!

@eddyb eddyb force-pushed the eddyb:back-to-roots branch from b1d2f75 to fb3c098 Jun 26, 2016
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jun 26, 2016

change_parentage is #11703.

@eddyb

This comment has been minimized.

Copy link
Contributor Author

eddyb commented Jun 26, 2016

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

bors added a commit to rust-lang/rust that referenced this pull request Jun 27, 2016
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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 4, 2016

📌 Commit 9584669 has been approved by Ms2ger

@highfive highfive assigned Ms2ger and unassigned jdm Jul 4, 2016
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 4, 2016

⌛️ Testing commit 9584669 with merge 162af13...

bors-servo added a commit that referenced this pull request Jul 4, 2016
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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11872)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 4, 2016

💔 Test failed - linux-dev

@eddyb eddyb force-pushed the eddyb:back-to-roots branch from 9584669 to b79a7d4 Jul 4, 2016
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jul 4, 2016

@bors-servo: r=Ms2ger

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 4, 2016

📌 Commit b79a7d4 has been approved by Ms2ger

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 4, 2016

⌛️ Testing commit b79a7d4 with merge 80cb0cf...

bors-servo added a commit that referenced this pull request Jul 4, 2016
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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11872)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 4, 2016

@bors-servo bors-servo merged commit b79a7d4 into servo:master Jul 4, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Jul 4, 2016
4 of 4 tasks complete
@eddyb eddyb deleted the eddyb:back-to-roots branch Jul 4, 2016
bors added a commit to rust-lang/rust that referenced this pull request Jul 6, 2016
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.
nox added a commit that referenced this pull request Aug 13, 2016
We don't use it anymore since #11872.
samuknet added a commit to samuknet/servo that referenced this pull request Sep 6, 2016
We don't use it anymore since servo#11872.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.