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

Wrong ABI size for imported enum #24589

Open
arnetheduck opened this issue Dec 29, 2024 · 11 comments
Open

Wrong ABI size for imported enum #24589

arnetheduck opened this issue Dec 29, 2024 · 11 comments

Comments

@arnetheduck
Copy link
Contributor

arnetheduck commented Dec 29, 2024

Description

Importing enum into Nim is broadly a bad idea given the semantic differences, but it's done anyway - when doing so, Nim should respect C enum ABI rules - in particular, an enum is sized as cint by default most of the time but not always.

type X {.importc: "someenum".} = enum
  ...

Failing to do so leads silent miscompiles (and fails -d:checkAbi)

Nim Version

devel

Current Output

No response

Expected Output

No response

Known Workarounds

No response

Additional Information

No response

@arnetheduck
Copy link
Contributor Author

See https://github.com/nim-lang/Nim/actions/runs/12533490011/job/34953222171#step:11:13480 for an example, where MemoryOrder has the wrong size.

@Araq
Copy link
Member

Araq commented Dec 29, 2024

Well c2nim adds .size: sizeof(cint) for these and so should we in our own stdlib.

@arnetheduck
Copy link
Contributor Author

Well c2nim adds .. so should we

well, this is a part of the semantic gap that importc could gracefully deal with - it's wrong every time to use nim semantics for c types.

@Araq
Copy link
Member

Araq commented Dec 29, 2024

It is not wrong if the C type ends up to be the type Nim chooses (usually byte), in C++ you can set the size and in C11 maybe too. So this would be a breaking change, silently changing the meaning even.

@arnetheduck
Copy link
Contributor Author

arnetheduck commented Dec 29, 2024

So, what c2nim does is wrong also since the size is implementation-defined in C11 and cannot be specified explicitly - the (most) correct thing to do here is to emit code based on knowledge about the specific compiler and platform or not allow sizeof or alignment-based operations on any type that contains an importc enum (just like importc struct without completeStruct).

GCC for example: https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html#Structures-unions-enumerations-and-bit-fields-implementation

It's the middle ground here that is wrong, ie where nim asserts that it knows the size but then emits code that does not match what the c compiler does.

Here's a trivial example of invalid codegen:

import std/atomics

type MyType = object
  v: MemoryOrder
  field: byte


echo sizeof(MyType)

this prints 2 - the real size of MyType is 8 on gcc/linux/x86_64 (but not on all platforms!).

@Araq
Copy link
Member

Araq commented Dec 29, 2024

Here's a trivial example of invalid codegen

Yes but we already do agree that eventually MemoryOrder is wrong. We disagree whether the bugfix should be in its declaration or in the compiler.

@arnetheduck
Copy link
Contributor Author

We disagree

Well, the size: sizeof(cint) solution disagrees with the guarantees given by the relevant standards so it's a non-starter. That leaves "size: unknown" or "size: messyPlatformFunction()" on the table.

@Araq
Copy link
Member

Araq commented Dec 29, 2024

ANSI C says an enum is of type int so I fail to see how we violate the standard.

@arnetheduck
Copy link
Contributor Author

arnetheduck commented Dec 29, 2024

C99 / C11 standards:

6.7.2.2 Enumeration specifiers

Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined, but shall be capable of representing the values of all the members of the enumeration.

the enum values/constants are of type int but the storage type (what happens with the values when they're stored in memory, and how they're packed) is implementation-defined (the compiler is allowed to do an implicit narrowing conversion).

@Araq
Copy link
Member

Araq commented Dec 29, 2024

Thanks, most interesting. The C compilers we officially support all map it to int though afaict.

@arnetheduck
Copy link
Contributor Author

The C compilers we officially support

except gcc, according to the link above, which lets the platform ABI dictate it - apparently used on some arm targets, but not all, and microcontrollers.

even worse, users can also change it with a c flag. it's a mess, which is why leaving it undefined so that sizeof doesn't work seems like the easiest option for now. It's not that much of a loss - nim mostly doesn't need it, except when it does (in low-level code like threads etc). giving an error here is better that silently miscomputing.

The final option is actually the "autoconf" approach more or less which is to compile a small program before the main compilation and thus find out "target information" from the underlying c compiler.

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

2 participants