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

mozjs_sys + target=android-i686: Rust compilation error #21093

Closed
paulrouget opened this issue Jun 25, 2018 · 15 comments
Closed

mozjs_sys + target=android-i686: Rust compilation error #21093

paulrouget opened this issue Jun 25, 2018 · 15 comments

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jun 25, 2018

./mach build --target=i686-linux-android -d

rustc 1.28.0-nightly (b68432d56 2018-06-12)
info: component 'rust-std' for target 'i686-linux-android' is up to date
   Compiling mozjs_sys v0.51.2
error[E0605]: non-primitive cast: `u64` as `[u32; 2]`
    --> /home/paul/git/servo/target/i686-linux-android/debug/build/mozjs_sys-6e56c52d2ad6e5a9/out/jsapi.rs:1633:7603
     |
1633 |  # [ repr ( C ) ] # [ derive ( Debug , Copy ) ] pub struct CallArgs { pub argv_ : * mut root::JS::Value , pub argc_ : :: std :: os :: raw :: c_uint , pub _bitfield_1 : root :: __BindgenBitfieldUnit
     |
     |
     = note: an `as` expression can only be used to convert between primitive types. Consider using the `From` trait
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jun 25, 2018

Message appeared after 2544db1

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jun 25, 2018

@asajeffrey @jdm is away. Maybe you can help.

@paulrouget paulrouget changed the title mozjs_sys + target=android=i686: Rust compilation error mozjs_sys + target=android-i686: Rust compilation error Jun 25, 2018
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jun 25, 2018

I think there may be a rustc bug causing the error message to show the wrong span of source, and a bindgen bug causing an invalid cast to be emitted. It would help to look at the generated jsapi.rs file to see what u64 value is cast to [u32; 2] with the as operator and why.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jun 25, 2018

Not sure where this is failing. Can't find an obvious culprit: https://gist.github.com/paulrouget/1700b9da726208d4022f1fe2e4ecb610

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 25, 2018

Does mach build --android work?

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 25, 2018

Also, you may be picking up an old jsapi.rs, try removing it and see if that helps.

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jun 25, 2018

impl < AllocPolicy > HashTable < AllocPolicy > { 
  # [ inline ] pub fn gen ( & self ) -> u64 { 
    unsafe { :: std :: mem :: transmute ( 
      self . _bitfield_1 . get ( 0usize , 56u8 ) as [ u32 ; 2usize ] ) } }
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 25, 2018

Which in my copy of jsapi.rs (generated on and for linux) is:

                pub fn gen(&self) -> u64 {
                    unsafe { ::std::mem::transmute(self._bitfield_1.get(0usize, 56u8) as u64) }
                }

@emilio any ideas what's going on here?

@emilio
Copy link
Member

@emilio emilio commented Jun 25, 2018

Yeah, this is because uint64_t is 4-byte aligned in that arch. This is a bindgen bug since it's not supposed to care about alignment in this case, but I couldn't repro with:

struct Table
{
  unsigned long long foo: 56;
  unsigned long long bar: 8;
};

Which generates the following for ./target/debug/bindgen test.h -- -x c++ --target=android-i686:

/* automatically generated by rust-bindgen */

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct Table {
    pub _bindgen_opaque_blob: [u32; 2usize],
}
#[test]
fn bindgen_test_layout_Table() {
    assert_eq!(
        ::std::mem::size_of::<Table>(),
        8usize,
        concat!("Size of: ", stringify!(Table))
    );
    assert_eq!(
        ::std::mem::align_of::<Table>(),
        4usize,
        concat!("Alignment of ", stringify!(Table))
    );
}

So a test-case for this would be extremely helpful.

emilio added a commit to emilio/rust-bindgen that referenced this issue Jun 25, 2018
blob() does care about alignment, so we can get into an architecture where there
are integers where size != align.

This is a tentative fix for servo/servo#21093, though
as mentioned there I couldn't find a repro on my machine.
@emilio
Copy link
Member

@emilio emilio commented Jun 25, 2018

https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/js/public/HashTable.h#1224 is the relevant data structure if I'm not wrong... rust-lang/rust-bindgen#1339 is a potential fix for this, though a repro would be nice.

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 28, 2018

@emilio Hmm, running test.h through bindgen without the --target flag produces:

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct Table {
    pub _bitfield_1: __BindgenBitfieldUnit<[u8; 8usize], u64>,
    pub __bindgen_align: [u64; 0usize],
}
#[test]
fn bindgen_test_layout_Table() {
    assert_eq!(
        ::std::mem::size_of::<Table>(),
        8usize,
        concat!("Size of: ", stringify!(Table))
    );
    assert_eq!(
        ::std::mem::align_of::<Table>(),
        8usize,
        concat!("Alignment of ", stringify!(Table))
    );
}
impl Table {
    #[inline]
    pub fn foo(&self) -> ::std::os::raw::c_ulonglong {
        unsafe { ::std::mem::transmute(self._bitfield_1.get(0usize, 56u8) as u64) }
    }
    #[inline]
    pub fn set_foo(&mut self, val: ::std::os::raw::c_ulonglong) {
        unsafe {
            let val: u64 = ::std::mem::transmute(val);
            self._bitfield_1.set(0usize, 56u8, val as u64)
        }
    }
    #[inline]
    pub fn bar(&self) -> ::std::os::raw::c_ulonglong {
        unsafe { ::std::mem::transmute(self._bitfield_1.get(56usize, 8u8) as u64) }
    }
    #[inline]
    pub fn set_bar(&mut self, val: ::std::os::raw::c_ulonglong) {
        unsafe {
            let val: u64 = ::std::mem::transmute(val);
            self._bitfield_1.set(56usize, 8u8, val as u64)
        }
    }
    #[inline]
    pub fn new_bitfield_1(
        foo: ::std::os::raw::c_ulonglong,
        bar: ::std::os::raw::c_ulonglong,
    ) -> __BindgenBitfieldUnit<[u8; 8usize], u64> {
        let mut __bindgen_bitfield_unit: __BindgenBitfieldUnit<[u8; 8usize], u64> =
            Default::default();
        __bindgen_bitfield_unit.set(0usize, 56u8, {
            let foo: u64 = unsafe { ::std::mem::transmute(foo) };
            foo as u64
        });
        __bindgen_bitfield_unit.set(56usize, 8u8, {
            let bar: u64 = unsafe { ::std::mem::transmute(bar) };
            bar as u64
        });
        __bindgen_bitfield_unit
    }
}

which includes the foo and bar methods that cause the problem. These methods aren't generated by the command-line bindgen, but apparently are when servo uses bindgen. Is there a flag to bindgen to get it to treat a type as a bitfield rather than an opaque.

Also, the opaque type generated is [u32; 2usize] which looks wrong to me, shouldn't it be [u32; 4usize]?

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 28, 2018

Looks like we can work round this, by declaring js::detail::HashTable.* to be opaque. I'll submit a PR doing this.

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 28, 2018

I have a branch with the workaround: https://github.com/asajeffrey/mozjs/tree/opaque-hashtable

@paul: can you check to see if using this fixes your problem?

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 28, 2018

Submitted workaround PR servo/mozjs#141

bors-servo added a commit to servo/mozjs that referenced this issue Jun 28, 2018
Make  Make js::detail::HashTable.* opaque

This is a workaround for servo/servo#21093.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/mozjs/141)
<!-- Reviewable:end -->
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jun 29, 2018

@asajeffrey this fixes it! Thank you.

@asajeffrey asajeffrey mentioned this issue Jun 29, 2018
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jun 29, 2018
Updated mozjs_sys to v0.51.3

<!-- Please describe your changes on the following line: -->

Allows building servo for target `i686-linux-android`.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #21093
- [X] These changes do not require tests because deperndency update

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21102)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 30, 2018
Updated mozjs_sys to v0.51.3

<!-- Please describe your changes on the following line: -->

Allows building servo for target `i686-linux-android`.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #21093
- [X] These changes do not require tests because deperndency update

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21102)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 30, 2018
Updated mozjs_sys to v0.51.3

<!-- Please describe your changes on the following line: -->

Allows building servo for target `i686-linux-android`.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #21093
- [X] These changes do not require tests because deperndency update

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21102)
<!-- Reviewable:end -->
emilio added a commit to emilio/rust-bindgen that referenced this issue Jul 5, 2018
blob() does care about alignment, so we can get into an architecture where there
are integers where size != align.

This is a tentative fix for servo/servo#21093, though
as mentioned there I couldn't find a repro on my machine.
bors-servo added a commit to rust-lang/rust-bindgen that referenced this issue Jul 6, 2018
Fix integer_type to actually return integers all the time.

blob() does care about alignment, so we can get into an architecture where there
are integers where size != align.

This is a tentative fix for servo/servo#21093, though
as mentioned there I couldn't find a repro on my machine.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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