-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
return_address intrinsic problematic with MIR and hard/impossible to use safely. #34227
Comments
The problematic function is The problematic code is pub fn new<T: CallbackContainer>(callback: &T, handling: ExceptionHandling) -> CallSetup {
let global = unsafe { global_root_from_object(callback.callback()) };
let cx = global.r().get_cx();
let exception_compartment = unsafe {
GetGlobalForObjectCrossCompartment(callback.callback())
};
CallSetup {
exception_compartment: RootedObject::new_with_addr(cx,
exception_compartment,
unsafe { return_address() }),
cx: cx,
old_compartment: unsafe { JS_EnterCompartment(cx, callback.callback()) },
handling: handling,
}
} RVO is actually pretty reliable with MIR (in initializations, that's it, not in assignments) - the only problem is that there is an aggregate here, and that we don't have aggregate RVO optimization. |
Well, it looks like |
The return_address trick is a performance optimization to avoid heap allocations. Strictly speaking, RootedVec isn't necessary at all: a It would be easy to change RootedVec so it contains a |
@eefriedman I think the last option is the best one, and it can still use |
FYI, there's a couple more uses of return_address in rust-mozjs: https://github.com/servo/rust-mozjs/blob/c6f6817a7beb7f310050e1dde88654a95de6df26/src/rust.rs . In particular, I think there's a bunch of code in Servo using the Rooted type. |
@eefriedman Ah, there it is! |
Well, the root set needs to know the length too so just keeping a ptr to the heap alloc isn't enough. What we can do is have: struct RootedVecBox<T> {
len: usize,
cap: usize,
rootsetptr: *const RootSetEntry,
data: [T]
} and use |
@Manishearth However, |
|
@jdm Are both in active use? I'm envisioning something like this would be optimal before stack maps: #[thread_local]
static ROOT_STACK: RootStack = RootStack {
roots: [Cell::new(ptr::null()); 32 * 1024],
top: Cell::new(0)
}; When pushing, check for overflow, increment the top, store your pointer and keep the index. I believe For
Note that |
They are both in active use, yes. We use |
On IRC @nox brought up that /*
* Moving GC Stack Rooting
*
* A moving GC may change the physical location of GC allocated things, even
* when they are rooted, updating all pointers to the thing to refer to its new
* location. The GC must therefore know about all live pointers to a thing,
* not just one of them, in order to behave correctly.
*
* The |Rooted| and |Handle| classes below are used to root stack locations
* whose value may be held live across a call that can trigger GC. For a
* code fragment such as:
*
* JSObject* obj = NewObject(cx);
* DoSomething(cx);
* ... = obj->lastProperty();
*
* If |DoSomething()| can trigger a GC, the stack location of |obj| must be
* rooted to ensure that the GC does not move the JSObject referred to by
* |obj| without updating |obj|'s location itself. This rooting must happen
* regardless of whether there are other roots which ensure that the object
* itself will not be collected.
*
* If |DoSomething()| cannot trigger a GC, and the same holds for all other
* calls made between |obj|'s definitions and its last uses, then no rooting
* is required. This does not invalidate my shadow stack suggestion, but it does require holding a reference to an element of the thread-local array (i.e. EDIT: There's, however, the problem of hooking SM for the moving GC, not just tracing. |
If you wanted to go the slightly awkward route, it's possible to write a macro that constructs a Rooted on the stack correctly: rooted!(let data = RootedValue::new(global.get_cx(), init.data));
// Expands to:
let data = RootedValueContainer::new(global.get_cx(), init.data);
let data = RootedValue::new(&mut data); |
@eefriedman I've been told that was tried and didn't scale. I think the shadow stack can be more efficient than the linked list shenanigans and might eventually end up being mutated into the optimal stack map integration. |
compiler team would like more detail from servo if possible about exactly what guarantees they expect from the compiler (in terms of extra moves generated by mir-based trans vs old-trans)... |
(also: not sure if best fix possible is on the compiler side) Anyway: assigning P-high because the breakage here breaks Servo. |
I can experiment with an approach like the TLS shadow stack made out of @jdm @Manishearth @pcwalton What's a reliable way to measure performance of SM <-> Rust interactions, i.e. can I tell whether I've made rooting slower? |
Probably running the dromaeo tests; |
The bad news: TLS gets ugly and has some overhead on platforms without The good news: I have originally misjudged the scale that I was able to replace most uses with a macro which defines two variables, one being the stable object which has to be referenced from some list of roots and the other one the guard that does the insertion/removal from the list of roots, and provides access to the rooted value. That macro doesn't work in 1.9 or earlier due to a hygiene bug which got fixed for 1.10, so my guess right now is that what I did has been tried in the past, unsuccessfully, due to the hygiene bug. I will post the Servo PR with the performance numbers when I get |
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 -->
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 -->
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.
Replace return_address usage in Rooted with a stack guard and a rooted! macro. Part of a potential solution for rust-lang/rust#34227. <!-- Reviewable:start --> --- 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) <!-- Reviewable:end -->
Since servo was moved away from return_address and we removed the intrinsic in question this can be closed? |
@nagisa It's not yet fully merged. |
Replace return_address usage in Rooted with a stack guard and a rooted! macro. Part of a potential solution for rust-lang/rust#34227. <!-- Reviewable:start --> --- 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) <!-- Reviewable:end -->
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 -->
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 -->
…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
…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
…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
Servo currently can't compile with
-Zorbit
(see servo/servo#11706) and my suspect isreturn_address
.Even if that wasn't the reason, Servo's usage of
return_address
to create a rooting cycle only ever worked because it happened to get the final destination of the call, but with MIR, the call destination might be a temporary, so it's not the final destination anymore.Servo has a lint for disallowing moves on the types that are being returned, but that analysis is wrong unless performed on exactly the MIR we translate.
We also could change the ABI at any time, so the indirect returns becomes direct (we can do this for any pair of pointer-sized values) which would also break Servo.
I'm not even sure how LLVM doesn't miscompile what Servo is doing, capturing an
sret
argument seems quite dubious to me, and some architectures also have an intermediary copy.AFAIK, the eventual solution is to have proper stack map support, however that might be a long ways off and I don't want to block removing old trans on it.
cc @jdm @pcwalton @Manishearth @rust-lang/compiler
The text was updated successfully, but these errors were encountered: