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

c_bool is not (always?) c_int, causes miscompilation of extent hook API #53

Closed
Arnavion opened this issue May 4, 2023 · 6 comments · Fixed by #54
Closed

c_bool is not (always?) c_int, causes miscompilation of extent hook API #53

Arnavion opened this issue May 4, 2023 · 6 comments · Fixed by #54

Comments

@Arnavion
Copy link

Arnavion commented May 4, 2023

type c_bool = c_int;

This leads to a miscompilation when using API that involves c_bool, ie the extent hook API. For example, when my implementation of this Rust typedef wrote to the supposed c_bool pointers, it ended up smashing other variables in the caller's stack. I'm using the x86_64-unknown-linux-musl and x86_64-unknown-linux-gnu targets with gcc 11, gcc 12 and gcc 13.

jemalloc expects to be using C99 bool via the stdbool.h header, and the equivalent of that is Rust's bool, not c_int. jemalloc does have this header for Windows MSVC that defines it to win32's BOOL which is apparently an int. I'm not sure if even that is required, since web search seems to indicate MSVC also got stdbool.h in VS 2013, but I don't have Windows to check myself.


Workaround: Write the extent hook implementations with the correct signature (using Rust bool), then transmute then into the incorrect signatures that tikv-jemalloc-sys requires (using c_int) via std::mem::transmute.

@BusyJay
Copy link
Member

BusyJay commented May 8, 2023

How about defining c_bool conditionally? If it's on MSVC windows, pick c_int, otherwise pick bool.

@Arnavion
Copy link
Author

Arnavion commented May 8, 2023

Did you confirm that building on MSVC does indeed use the compat header and not whatever stdbool.h MSVC (apparently) aleady ships with?

@BusyJay
Copy link
Member

BusyJay commented May 9, 2023

No, I don't have any machine that runs Windows. The file is named stdbool.h and get included in the header searching path by configure script, so even MSVC ships one, the former should be used instead.

@Arnavion
Copy link
Author

Arnavion commented May 9, 2023

Okay. Then yes, the fix would be:

#[cfg(target_env = "msvc")]
type c_bool = c_int;
#[cfg(not(target_env = "msvc"))]
type c_bool = bool;

@BusyJay
Copy link
Member

BusyJay commented May 10, 2023

Cool, can you send it as a PR?

@Arnavion
Copy link
Author

#54

BusyJay pushed a commit that referenced this issue May 10, 2023
Fixes #53

Signed-off-by: Arnav Singh <me@arnavion.dev>
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 a pull request may close this issue.

2 participants