Skip to content

Bools should be a single byte #1076

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

Closed
thomcc opened this issue Mar 14, 2019 · 14 comments
Closed

Bools should be a single byte #1076

thomcc opened this issue Mar 14, 2019 · 14 comments

Comments

@thomcc
Copy link

thomcc commented Mar 14, 2019

See also #471. There are still several places in the C++ code that seem to assume boolean is 4 bytes. On every ABI I'm aware of that's still in use, C's _Bool and C++'s bool are 1 byte.

This has caused numerous problems for us in the past, and so we don't use booleans with JNA (and just return explicitly sized 1/0 integers), but for some reason I thought you had an issue for this open already.

@matthiasblaesing
Copy link
Member

The difficult part will be to do this in a manner, that either breaks existing code, so that broken behaviour is immediately visible or backwards compatible. At least in the platform I'm pretty sure that there are multiple places, that expect boolean to be typedef of int.

@thomcc
Copy link
Author

thomcc commented Mar 14, 2019

That's unfortunate (although I figured as much, to be honest).

One option would be to remove support for boolean, which would break existing code (although I can see why you wouldn't want to do that).

At a minimum this should be documented, IMO (for example, the way NativeSize is called out in your docs).

@matthiasblaesing
Copy link
Member

Please see http://java-native-access.github.io/jna/5.2.0/javadoc/ section Marshalling/Unmarshalling (Java/Native Type Conversions) -> it is documented.

@thomcc
Copy link
Author

thomcc commented Mar 14, 2019

Ah, yeah, alright. Well, I still think this is a bug that should be fixed, but I guess I glossed over that it was in the docs.

@matthiasblaesing
Copy link
Member

As said: If you can come up with a plan to introduce this, I'll help.

@matthiasblaesing
Copy link
Member

matthiasblaesing commented Mar 20, 2019

Here is a sample, that boolean -> int8_t typemapping works (int8_t as a placeholder for a 1 byte wide boolean):
matthiasblaesing@f15508c
it works for normal interface method and direct mapping.

@matthiasblaesing
Copy link
Member

Seeing no further reaction I'm closing this.

@blastrock
Copy link

Hi,

I also stumbled upon this bug. For me, it happened only on an x86_64 android emulator when my library was compiled in release mode. I lost two days fighting against adb, lldb, and android studio before reaching the problematic line and then this issue.

I understand that this is documented and breaking it may break users' code, but I think something should be done for this issue.

As said: If you can come up with a plan to introduce this, I'll help.

One way you could go about this is to add an option to Native.loadLibrary. If the option is not set, you could print a deprecation warning (or throw). If the option is false, you continue to implement the old behavior without warning. If it is true, you implement the new one complying with the ABI rules for C99's _Bool and C++'s bool.

@thomcc
Copy link
Author

thomcc commented Feb 5, 2020

Yeah this is... still broken. I'm not sure what exactly you're looking for here as for a plan in #1076 (comment)

We've just been avoiding using bool at all, which is painful since it's easy to forget (hence the most recent reference on this issue). It would be better if this issue were just fixed so that it wouldn't be so much of a footgun.

I'm sympathetic to the fact that this is a breaking change (and that a lot of pre-c99 libraries say bool but just use an enum or a typedef over int) but... yeah, it's frustrating.

It's worth noting that the place this comes up the most is when using bool as a struct field, for arguments and return values the calling convention is usually (but IIRC not always) the same between bool and int.

@matthiasblaesing
Copy link
Member

@thomcc the issue with backwards compatibility is very real. Imagine, that you have bindings that map windows BOOL (https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types). That is a typedef to integer. Yes it is broken, but this being part of the API will not go away. So I could now create bindings and map the win32 functions to bool and I expect JNA not to break that expectation when I go from JNA 5 to 6.

This is all, that is needed to use a boolean that is typdeffed to int8_t:

  1. Define a typemapper, that does the "right" thing for you:
    matthiasblaesing@f15508c#diff-d3ad5b031097fbb9458e583324d1b552R102-R128

  2. Pass that typemapper to the library binding:
    matthiasblaesing@f15508c#diff-d3ad5b031097fbb9458e583324d1b552R35
    matthiasblaesing@f15508c#diff-d3ad5b031097fbb9458e583324d1b552R46

IMHO trading backwards compatibility and lurking breakage is not worth it, considering the easy fix. Changing the mapping of java boolean from int32_t to int8_t will only show at runtime and then with random segfaults and such.

@matthiasblaesing
Copy link
Member

I just noticed, that not only win32 api maps BOOL to int, but gobject does so too (gboolean is typedeffed to gint, which is typdeffed to int):

https://developer.gnome.org/glib/stable/glib-Basic-Types.html#gboolean

@thomcc
Copy link
Author

thomcc commented Feb 15, 2020

Win32's BOOL is really an int though -- Common APIs like GetMessage require checking that it's not -1: (See the note about return value here: https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getmessage). Regardless of this issue, I'm not sure mapping that to a java boolean makes sense...

More broadly, yes, as I conceded, there are a lot of pre-C99 APIs that call the return value a boolean when using an integer. This is admittedly confusing, but there is a platform ABI, which defines the size of a boolean for all these platforms, and with remarkably few exceptions they all define it to be one byte.

Anyway, at one point I attempted the fix you describe at one point and still had issues with bools in struct fields?

@matthiasblaesing
Copy link
Member

This is admittedly confusing, but there is a platform ABI, which defines the size of a boolean for all these platforms, and with remarkably few exceptions they all define it to be one byte.

That argument does not hold. Java char is not mapped to C char (it is a wchar), Java long maps to long long and for a native C long you'll need NativeLong. So it is an illusion, that the java types map directly to C types.

Anyway, at one point I attempted the fix you describe at one point and still had issues with bools in struct fields?

You need to be more concrete to get an answer. I wipped up a quick test for a struct with a stdbool member and for me it works:

matthiasblaesing@0b8d10e

@matthiasblaesing
Copy link
Member

Could we move this please to the mailing list? The discussion here might interest others.

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

No branches or pull requests

3 participants