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(ext/ffi): Use c_char instead of i8 in op_ffi_cstr_read for compatibility with aarch64 #13118

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

LukeChannings
Copy link
Contributor

👋

This patch fixes a build error I get when compiling for aarch64 in Linux:

Compiling deno_ffi v0.16.0 (/deno/ext/ffi)
error[E0308]: mismatched types
   --> ext/ffi/lib.rs:612:30
    |
612 |   Ok(unsafe { CStr::from_ptr(ptr) }.to_str()?.to_string())
    |                              ^^^ expected `u8`, found `i8`
    |
    = note: expected raw pointer `*const u8`
               found raw pointer `*const i8`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `deno_ffi` due to previous error
warning: build failed, waiting for other jobs to finish...
error: build failed

This is caused by CStr::from_ptr returning an i8 on aarch64, instead of a u8.
Using std::os::raw::c_char will handle the underlying type differences (I think).

I've compiled with this patch here and it compiles correctly for aarch64.

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2021

CLA assistant check
All committers have signed the CLA.

@LukeChannings LukeChannings changed the title fix(ext/ffi): Use c_char instead of i8 in op_ffi_cstr_read for compatibility with aarch64 fix(ext/ffi): Use c_char instead of u8 in op_ffi_cstr_read for compatibility with aarch64 Dec 17, 2021
@LukeChannings LukeChannings changed the title fix(ext/ffi): Use c_char instead of u8 in op_ffi_cstr_read for compatibility with aarch64 fix(ext/ffi): Use c_char instead of i8 in op_ffi_cstr_read for compatibility with aarch64 Dec 17, 2021
@crowlKats
Copy link
Member

CI is failing due to bad formatting, could you please run tools/format.js?

@LukeChannings
Copy link
Contributor Author

@crowlKats Sorry about that, it passes formatting / linting locally now and I've pushed an update.

Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

LGTM

@LukeChannings
Copy link
Contributor Author

cc. @eliassjogreen since this is his code.

Copy link
Contributor

@eliassjogreen eliassjogreen left a comment

Choose a reason for hiding this comment

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

LGTM

@crowlKats crowlKats merged commit e5a8588 into denoland:main Dec 17, 2021
@LukeChannings LukeChannings deleted the bugfix/i8-aarch64-ffi branch December 17, 2021 09:43
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.

4 participants