servo / rust-mozjs

DEPRECATED - moved to servo/mozjs instead.
Mozilla Public License 2.0
293 stars 122 forks source link

Remove the drop flags that can be removed #227

Closed nox closed 8 years ago

nox commented 8 years ago

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

Cc @ecoal95

Review on Reviewable

michaelwu commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

michaelwu commented 8 years ago

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. https://github.com/servo/mozjs/pull/55 has the changes I made to the headers, so you can see some examples there.

Manishearth commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

See https://github.com/michaelwu/rust-bindgen/pull/7 for another take at this.

nox commented 8 years ago

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

michaelwu commented 8 years ago

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.