Remove the drop flags that can be removed

[nox]

The remaining one is the one on Heap<T>. This should fix the issue related to #226.

Cc @ecoal95

[michaelwu]

Directly modifying the jsapi_*.rs files is generally not allowed - these files are fully autogenerated. You'll want to either make all generated structs have unsafe_no_drop_flag, add an annotation to mark certain structs as unsafe_no_drop_flag, or figure out some other way to convince rust not to add a drop flag (mark things as nonzero upstream and have bindgen wrap things in NonZero, maybe?).

[nox]

@michaelwu Where should that be done? In your bindgen's fork? I suspected such a thing for starters, but I wanted to at least make a PR to know if that is actually a fix for #226.

[nox]

On a related note, IMO that unsafe_no_drop_flag should actually live on the Drop impl.

[michaelwu]

You'll most likely need to hack bindgen. You may also want to add additional things to the upstream headers. I like the idea of wrapping things in NonZero if that'll make rust automatically remove the drop flag. servo/mozjs#55 has the changes I made to the headers, so you can see some examples there.

[emilio]

Heh, I should have tried to write this. In my defence I was on a bus while testing.

I think this won't work, since rust fills the dropped objects (even those marked with #[unsafe_no_drop_flag]) with 0x1d (see https://github.com/rust-lang/rust/blob/040a77f772d7693598499a161f53ed230fb61c52/src/libcore/mem.rs#L539).

A quick test with this patch applied over #227 yields:

(gdb) run
Starting program: /home/emilio/projects/moz/rust-mozjs/target/debug/js-d2b536494d16a8fc 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

running 2 tests
[New Thread 0x7ffff6b31700 (LWP 17770)]
[New Thread 0x7ffff5fff700 (LWP 17771)]
[New Thread 0x7ffff57ff700 (LWP 17772)]
[New Thread 0x7ffff4fff700 (LWP 17773)]
[New Thread 0x7ffff7fc0700 (LWP 17774)]
[New Thread 0x7ffff7f3f700 (LWP 17775)]
[New Thread 0x7ffff7ebe700 (LWP 17776)]
[New Thread 0x7ffff6930700 (LWP 17777)]
[New Thread 0x7ffff68af700 (LWP 17778)]
[New Thread 0x7ffff5dfe700 (LWP 17779)]
[New Thread 0x7ffff5d7d700 (LWP 17780)]
[New Thread 0x7ffff5cfc700 (LWP 17781)]
[New Thread 0x7ffff55fe700 (LWP 17782)]
[New Thread 0x7ffff557d700 (LWP 17783)]
[New Thread 0x7ffff54fc700 (LWP 17784)]
[New Thread 0x7ffff4dfe700 (LWP 17785)]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff57ff700 (LWP 17772)]
0x000055555563bc7c in js::rust::Rooted<T>.Drop::drop (self=0x7ffff57fe980) at src/rust.rs:337
337             assert!(*self.stack == mem::transmute(&*self));
(gdb) p self.stack
$1 = (struct Rooted<*mut libc::c_void> **) 0x1d1d1d1d1d1d1d1d
(gdb) p self
$2 = (struct Rooted<*mut js::jsapi_linux_64::JSObject> *) 0x7ffff57fe980
(gdb) p self._base
$3 = {_phantom0 = {<No data fields>}}
(gdb) p self.prev
$4 = (struct Rooted<*mut libc::c_void> *) 0x1d1d1d1d1d1d1d1d
(gdb) p self.ptr
$5 = (union JSObject *) 0x1d1d1d1d1d1d1d1d

We might want to check for 0x1d1d1d (I just added a commit to #226 that does it, and works) , but it feels a bit too hacky...

[Manishearth]

I think this won't work, since rust fills the dropped objects (even those marked with #[unsafe_no_drop_flag]) with 0x1d

Wait, what?

The point of unsafe_no_drop_flag is that you're manually tracking the drop state somewhere, which can be inline. Won't the drop fill interact badly with it? I'd expect it not to happen for unsafe_no_drop_flag.

@asajeffrey was working with this attribute, maybe he knows how drop fill interacts with it.

[emilio]

@Manishearth My fault, seems that the filling is done when contained in ForOfIterator (that in my experiment had no #[unsafe_no_drop_flag]). Adding that to ForOfIterator too in my patch would suffice, but we'll have to be careful when containing Rooteds in other structs (if that can be the case).

Example: https://play.rust-lang.org/?gist=517c053e0daf7b56e6b9&version=nightly

[nox]

See michaelwu/rust-bindgen#7 for another take at this.

[nox]

@michaelwu What's blocking this now that it is rebased and that your fork of bindgen adds such drop flags?

[michaelwu]

Does it work? Did we have to switch to checking for the poison instead of null? Also, I assume you've verified that bindgen does in fact remove the drop flag for these structs.