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

Fix compilation for 32-bit targets #1050

Merged
merged 1 commit into from
Aug 21, 2022
Merged

Fix compilation for 32-bit targets #1050

merged 1 commit into from
Aug 21, 2022

Conversation

wg
Copy link
Contributor

@wg wg commented Aug 16, 2022

Hello, thank you for rusty_v8! Building for the 32-bit armv7-unknown-linux-musleabihf
target gives the following compilation error due to some assumptions about the
layout of the C++ v8::ScriptCompiler::Source class:

  ../../../../src/binding.cc:43:50: error: static assertion failed: Source size mismatch
     43 | static_assert(sizeof(v8::ScriptCompiler::Source) <= sizeof(size_t) * 8,

This assert matches the corresponding Rust struct definition pub struct Source([usize; 8]);
on 64-bit platforms where the size of the fields plus padding mean
sizeof(v8::ScriptCompiler::Source) == 64.

However on 32-bit ARM sizeof(v8::ScriptCompiler::Source) == 36 but
size_of::<[usize; 8]>() == 32.

This PR updates the layout of the Rust v8::script_compiler::Source struct
to match that of the C++ class and updates the static_assert to check that the size
of the C++ class matches the expected aligned sum of field sizes.

With this change I'm able to build & run a program that links to rusty_v8 on
armv7-unknown-linux-musleabihf, as well as aarch64-unknown-linux-musl, and
x86_64-unknown-linux-musl.

@CLAassistant
Copy link

CLAassistant commented Aug 16, 2022

CLA assistant check
All committers have signed the CLA.

@@ -61,7 +62,17 @@ extern "C" {
/// Source code which can then be compiled to a UnboundScript or Script.
#[repr(C)]
#[derive(Debug)]
pub struct Source([usize; 8]);
pub struct Source {
_source_string: usize,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentations don't look quite right to. You could fix these by running cargo fmt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've fixed this. And thank you for kicking off the workflow run @littledivy.

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@littledivy littledivy merged commit fa01b39 into denoland:main Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants