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

Upgrade to SM 39 #150

Merged
merged 18 commits into from Jun 19, 2015
Merged

Upgrade to SM 39 #150

merged 18 commits into from Jun 19, 2015

Conversation

@michaelwu
Copy link
Contributor

michaelwu commented May 21, 2015

Some things have been implemented in rust.rs. Where possible, things are being implemented using the structs generated by my bindgen fork (which I will call bindgen++ from now on).

jsapi.rs has been hand edited a bit. Note that jsapi.rs is generated from jsfriendapi.hpp so we can eliminate jsfriendapi.rs entirely. Making jsfriendapi.rs work with with jsapi.rs was a bit difficult, so I combined them to sidestep the problem.

Here are the things I had to hand edit in jsapi.rs:

  • Adding "unsafe impl Sync for " for structs that are statically defined.
  • Adding "pub enum Foo {}" for types not defined in js headers. (usually mfbt)
  • Adding "#[derive(Copy)]" where necessary to those defines.
  • Removing derive copy from autogenerated structs that don't support it because bindgen++ doesn't fully define all the structs it depends on.
  • Trimming away generics for structs that didn't get converted successfully. This can happen for a number of reasons:
    • bindgen++ can only convert templates that work on types
    • bindgen++ can't convert the classes that a struct/class might inherit from
  • 64 bit issues:
    • Certain types need to be converted back to size_t since bindgen++ currently replaces them with u32 or u64. This will be fixed.
    • Some mangled function names are different between 32 bit and 64 bit, usually because of 64 bit types and size_t/long. In these cases, functions are hand edited to add 64bit versions. Might be fixed in the future, but for now we can only detect these problems when code doesn't link.
  • Spidermonkey checks for the address of JS_GlobalObjectTraceHook in globals. For now, I made the mangled name public so we can grab the real function address, but we can probably do a little bit better.
  • References to JSVal are replaced with Value, since jsapi.rs and jsval.rs both define JSVal. In jsapi.rs, JSVal is just a typedef to Value, so we can drop JSVal in jsapi.rs.
  • bindgen++ doesn't support preprocessor defines at the moment, so some defines were manually added.
  • JSJitMethodCallArgs was manually defined.
  • JS_GetRuntime wasn't converted by bindgen++, so it was manually added. Need to investigate.
  • Structs with unions generated by bindgen++ are generally unusable for statically defined things, so the necessary structs were modified to be more friendly for that case.

#144

@jdm
Copy link
Member

jdm commented Jun 1, 2015

I haven't reviewed the changes here yet, but I suggest implementing Deref for Handle/HandleMut so we don't need to call get() everywhere in the related Servo changes.

@Ms2ger
Copy link
Collaborator

Ms2ger commented Jun 15, 2015

Why does MutableHandle::set take &mut self?

@michaelwu
Copy link
Contributor Author

michaelwu commented Jun 15, 2015

It doesn't need to. I'll fix it.

@michaelwu
Copy link
Contributor Author

michaelwu commented Jun 15, 2015

char16_t will be converted correctly next time - michaelwu/rust-bindgen@b3069b1

@michaelwu michaelwu force-pushed the smupgrade2 branch from 8bc4065 to 6f1efbd Jun 15, 2015
@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented on src/rust.rs in a0341d5 Jun 18, 2015

Scary! But OK.

@pcwalton
Copy link
Contributor

pcwalton commented Jun 18, 2015

r=me with one or two tiny nits, gogogogo.

@michaelwu
Copy link
Contributor Author

michaelwu commented Jun 18, 2015

I'd like to have an attribute in rust that would let you specify the mangled name of an export symbol. This would let us support MSVC mangling, which uses things like ? and @ and $ and other crazy characters.

@michaelwu
Copy link
Contributor Author

michaelwu commented Jun 18, 2015

About the things in jsglue.cpp - I'm adding more support for C++ things in bindgen++ which will reduce the number of things in jsglue.cpp, possibly eliminating it altogether at some point.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jun 18, 2015

@bors-servo: r=pcwalton

@metajack
Copy link
Contributor

metajack commented Jun 18, 2015

@bors-servo r=pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Jun 18, 2015

📌 Commit 6f1efbd has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Jun 18, 2015

Testing commit 6f1efbd with merge 37f96fb...

bors-servo pushed a commit that referenced this pull request Jun 18, 2015
Upgrade to SM 39

Some things have been implemented in rust.rs. Where possible, things are being implemented using the structs generated by my bindgen fork (which I will call bindgen++ from now on).

jsapi.rs has been hand edited a bit. Note that jsapi.rs is generated from jsfriendapi.hpp so we can eliminate jsfriendapi.rs entirely. Making jsfriendapi.rs work with with jsapi.rs was a bit difficult, so I combined them to sidestep the problem.

Here are the things I had to hand edit in jsapi.rs:
- Adding "unsafe impl Sync for " for structs that are statically defined.
- Adding "pub enum Foo {}" for types not defined in js headers. (usually mfbt)
- Adding "#[derive(Copy)]" where necessary to those defines.
- Removing derive copy from autogenerated structs that don't support it because bindgen++ doesn't fully define all the structs it depends on.
- Trimming away generics for structs that didn't get converted successfully. This can happen for a number of reasons:
  - bindgen++ can only convert templates that work on types
  - bindgen++ can't convert the classes that a struct/class might inherit from
- 64 bit issues:
  - Certain types need to be converted back to size_t since bindgen++ currently replaces them with u32 or u64. This will be fixed.
  - Some mangled function names are different between 32 bit and 64 bit, usually because of 64 bit types and size_t/long. In these cases, functions are hand edited to add 64bit versions. Might be fixed in the future, but for now we can only detect these problems when code doesn't link.
- Spidermonkey checks for the address of JS_GlobalObjectTraceHook in globals. For now, I made the mangled name public so we can grab the real function address, but we can probably do a little bit better.
- References to JSVal are replaced with Value, since jsapi.rs and jsval.rs both define JSVal. In jsapi.rs, JSVal is just a typedef to Value, so we can drop JSVal in jsapi.rs.
- bindgen++ doesn't support preprocessor defines at the moment, so some defines were manually added.
- JSJitMethodCallArgs was manually defined.
- JS_GetRuntime wasn't converted by bindgen++, so it was manually added. Need to investigate.
- Structs with unions generated by bindgen++ are generally unusable for statically defined things, so the necessary structs were modified to be more friendly for that case.

#144
@bors-servo
Copy link
Contributor

bors-servo commented Jun 18, 2015

💔 Test failed - travis

@michaelwu michaelwu force-pushed the smupgrade2 branch from 6f1efbd to dfe8503 Jun 18, 2015
@michaelwu
Copy link
Contributor Author

michaelwu commented Jun 19, 2015

Directly merging for the same reason servo/mozjs#37 was directly merged - the build can't be completed in time before travis kills us.

michaelwu added a commit that referenced this pull request Jun 19, 2015
Upgrade to SM 39
@michaelwu michaelwu merged commit d0c4bd2 into master Jun 19, 2015
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
@Ms2ger Ms2ger deleted the smupgrade2 branch Jul 4, 2015
mmatyas pushed a commit to mmatyas/rust-mozjs that referenced this pull request Jul 30, 2015
Added a way to get the current point of the path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.