Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

TFBooleanType>>basicType should use uint8, not ushort #16

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

akgrant43
Copy link
Contributor

It looks like TFBooleanType is incorrectly defined:

TFBooleanType>>basicType

^ TFBasicType ushort

which is 16 bits, while booleans in C are 8 bits. (This caused one of our automated tests to fail. Interestingly, it only fails with the optimised version of the binary, the debug version passes).

Replacing it with:

TFBooleanType>>basicType

^ TFBasicType uint8

produces the correct behaviour.

@syrel
Copy link

syrel commented Nov 8, 2019

Confirmed on Mac too.

@estebanlm
Copy link
Member

real problem is: this depends largely on the library.
boolean in fact is a type that does not exists in C, but many libraries made some mapping to it.
That's why is a derived type and not a basic type.
I feel that we need to find a better solution here, because just replacing ushort with uint8 will probably fix your problem but It will break other places.

@syrel
Copy link

syrel commented Nov 10, 2019

Hi Esteban,

Indeed, boolean is not a basic type in C, however we are not talking about C here. In fact, libraries we use are not implemented in C nor C++. Let's instead take Application binary interface as a foundation for describing FFI types. For example both Alien and Squeak FFI should respect x86_64 SysV ABI according to for example the following issue on OpenSmalltalk.

SysV ABI is considered to be a de-facto standard when it comes to ABI, see a document here

On page 14 of the above mentioned document you can find a table of types and their corresponding size and alignment:
Screenshot 2019-11-08 at 10 57 14

C99's definition of boolean as _Bool is taken as a foundation for describing boolean type.

If for example SysV ABI and its boolean definition is not convincing enough we can look at another ABI specification, Itanium C++ ABI (also called Common Vendor ABI). In section 2.2 POD Data Types it defined a boolean as:

If the base ABI specifies rules for the C99 type _Bool, then bool follows those rules.
Otherwise, it has size and alignment 1.

where base ABI is System V ABI.

Enough about C 😃Let's talk Rust. Here is the official comment about bool size: rust-lang/rust#46176 (comment). In short:

  • bool has the same representation as the platform's _Bool type.
  • We document this, and also document that on every platform we currently support, this means that the size of bool is 1.

To conclude, the ideal solution would be to dispatch boolean size and alignment resolution through OSPlatform. Or agree that on all platforms supported by Pharo the size of bool is 1.

@akgrant43
Copy link
Contributor Author

This of course doesn't stop us from adding a bool16 type for any library that requires it.

@tesonep
Copy link
Collaborator

tesonep commented Dec 10, 2019

I like the fix right now, I would love to have an extension to choose the correct type in the library. I will open an issue for the future, but this fix is good enough right now.

@tesonep tesonep merged commit 3d0d420 into pharo-project:master Dec 10, 2019
@akgrant43 akgrant43 deleted the cBool branch December 10, 2019 09:14
@akgrant43
Copy link
Contributor Author

Thanks, Pablo!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants