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

Rust miscompiles Servo after upgrading to new revision of Rust #16366

Closed
zwarich opened this issue Aug 8, 2014 · 17 comments
Closed

Rust miscompiles Servo after upgrading to new revision of Rust #16366

zwarich opened this issue Aug 8, 2014 · 17 comments
Labels
P-medium Medium priority

Comments

@zwarich
Copy link

zwarich commented Aug 8, 2014

In the current Servo Rust upgrade branch (rustup-20140804-debug-segfault 4f40685a), Servo currently dies with a double borrow error loading essentially any page:

$ ./servo http://www.google.com
task 'LayoutWorker' failed at 'RefCell<T> already borrowed', /Users/jack/src/rust/src/libcore/cell.rs:306

Moving a single line of code out of an unsafe block makes this problem go away, which points to a miscompilation in at least one version of this code:

diff --git a/src/components/layout/layout_task.rs b/src/components/layout/layout_task.rs
index 632e3af..dd1bb09 100644
--- a/src/components/layout/layout_task.rs
+++ b/src/components/layout/layout_task.rs
@@ -573,9 +573,9 @@ impl LayoutTask {

     /// The high-level routine that performs layout tasks.
     fn handle_reflow(&mut self, data: &Reflow) {
+        let mut node: JS<Node> = unsafe { JS::from_trusted_node_address(data.document_root) };
         // FIXME: Isolate this transmutation into a "bridge" module.
         let node: &mut LayoutNode = unsafe {
-            let mut node: JS<Node> = JS::from_trusted_node_address(data.document_root);
             mem::transmute(&mut node)
         };
@larsbergstrom
Copy link
Contributor

There is a Servo branch that preserves this behavior at:
https://github.com/servo/servo/tree/rustup-20140804-debug-segfault

@larsbergstrom
Copy link
Contributor

@brson Can you help us find an owner for this? We landed a Rust upgrade by changing our code to avoid this compilation issue, so it's not blocking immediately, but still a scary bug.

@brson
Copy link
Contributor

brson commented Aug 12, 2014

Nominating.

@pnkfelix
Copy link
Member

Assigning P-high, not 1.0.

(Niko has volunteered to look into it further, which hopefully will yield more data for us to work with.)

@metajack
Copy link
Contributor

It's possible we just hit this again tracking down random graphics stack failures. See the Servo issue linked above.

@nikomatsakis
Copy link
Contributor

So I can certainly reproduce the failure. Investigating more.

@nikomatsakis
Copy link
Contributor

Reading this patch more clearly, I think the real problem is that the old code was just wrong and abusing transmute. I'm sure the crash is due to the new code that reuses stack space more aggressively. Take a look at what's going on here (original code, variables renamed for clarity):

     fn handle_reflow(&mut self, data: &Reflow) {
         // FIXME: Isolate this transmutation into a "bridge" module.
         let node2: &mut LayoutNode = unsafe {
             let mut node1: JS<Node> = JS::from_trusted_node_address(data.document_root);
             mem::transmute(&mut node1)
         };

Here a block is created and then a variable node1 inside that block. The address of node1 is taken and it is transmuted and stored in node2. But at that point the block terminates and hence the stack space for node1 is reclaimed. This should certainly not type check -- it actually might, but I suspect PR #16453 will fix it if it does, if not that's good to know -- but in any case here there is a transmute so the type checker is helpless.

@huonw
Copy link
Member

huonw commented Aug 16, 2014

(FWIW, that suggests that the bug (in Servo) may've been triggered by #15863, since that presumably allowed LLVM to start reusing the stack space of node1.)

@metajack
Copy link
Contributor

I'm not sure if this is related, but @zwarich and @mrobinson have been tracking down more bugs caused by the Rust upgrade and found this:

servo/servo#3084

@dotdash
Copy link
Contributor

dotdash commented Aug 16, 2014

Yep, similar bug there creating a dangling *mut pointer that points to a temporary in a match expression. Submitted a fix for that to rust-azure.

@metajack
Copy link
Contributor

Why does the println! affect it? Why does optimization affect it?

@dotdash
Copy link
Contributor

dotdash commented Aug 16, 2014

Rust is now making use of stack coloring, so stack slots may get reused
once a variable goes out of scope. This happens here and the unsafe code
creates a dangling pointer to a stack slot that is being reused. Stack
coloring only happens with optimizations enabled and adding the printf!
changes the stack usage and the resulting slot sharing from the stack
coloring pass. Therefore either of those can affect if and how such bugs
manifest
Am 16.08.2014 22:17 schrieb "Jack Moffitt" notifications@github.com:

Why does the println! affect it? Why does optimization affect it?


Reply to this email directly or view it on GitHub
#16366 (comment).

@nikomatsakis
Copy link
Contributor

@huonw yes, thanks for the link. That's the patch I was thinking of when I said "new code that reuses stack space more aggressively".

@luqmana
Copy link
Member

luqmana commented Aug 17, 2014

It seems like this isn't a rust bug so in that case can this be closed?

@zwarich
Copy link
Author

zwarich commented Aug 17, 2014

@luqmana Yes.

@zwarich zwarich closed this as completed Aug 17, 2014
@nikomatsakis
Copy link
Contributor

@metajack This kind of bug argues for servo not using the general purpose transmute, but rather more specific varieties. For example,a version that goes from &'a T to &'a U, and a similar version for &'a mut T to &'a mut U. That would have made this a compilation error rather than runtime crash.

@metajack
Copy link
Contributor

It seems like the second example could have been caught by the compiler, but I guess it stops all checking once you've cast to a raw pointer.

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 21, 2024
internal: Make data queries transparent over their diagnostics variant

And a few other QoL things
bors pushed a commit to rust-lang-ci/rust that referenced this issue Mar 17, 2024
PR rust-lang#16366 moved layout information to a separate line, so the
leading whitespace is no longer necessary.
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 17, 2024
minor: Fix unwanted leading whitespace in hover text

PR rust-lang#16366 moved layout information to a separate line, so the leading whitespace is no longer necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-medium Medium priority
Projects
None yet
Development

No branches or pull requests

9 participants