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

cgen incorrectly considers type to have C array semantics #1416

Open
shayanhabibi opened this issue Aug 14, 2024 · 2 comments
Open

cgen incorrectly considers type to have C array semantics #1416

shayanhabibi opened this issue Aug 14, 2024 · 2 comments

Comments

@shayanhabibi
Copy link
Collaborator

shayanhabibi commented Aug 14, 2024

Example

type
  int128 {.importc:"__int128".} = distinct array[2, uint64]
  hint128 = object
    hi, lo: int64

proc atomicAddFetch[int128](p: ptr int128, v: int128, mo = ATOMIC_SEQ_CST): int128 {.importc:"__atomic_add_fetch", nodecl.}

var atm = hint128(hi: 1'i64)
var res = cast[hint128](atomicAddFetch[int128](
  cast[ptr int128](addr atm), cast[int128](hint128(lo: 1'i64))
))
echo repr atm
echo repr res

provides the following compilation error: operand type 'NU64 (*)[2]' {aka 'long long unsigned int (*)[2]'} is incompatible with argument 1 of '__atomic_add_fetch'

see the following cgen

\\ ...
	_A2u64* _2;
	__int128 _3;
\\ ...
	nimCopyMem((void*)_5, (NIM_CONST void*)__atomic_add_fetch(_2, _3, __ATOMIC_SEQ_CST), 16);
\\ ...

This is corrected by changing the int128 definition to the following:

type
  int128 {.importc:"__int128".} = object
    t,b: int64
@zerbina
Copy link
Collaborator

zerbina commented Aug 14, 2024

There are two problems with the generated C code:

  1. the mentioned compilation error, caused by _2 being of type _A2u64* instead of __int128*
  2. the __int128 values are cast into pointers and then used as the source and destination of a memcpy, which is nonsense

Both problems stem from int128 being considered as having C array semantics by cgen (mapType returns ctArray for the int128).


Whether this behaviour is correct (as in, cgen does what it's told) is not entirely clear. On the surface, this could be considered a bug, but it's less clear when taking a deeper look. I think the more fundamental question that needs to be answered first is what type T {.importc: N.} = distinct U actually means.

The current meaning could be summarized as:

  • T is a type imported from C
  • T acts as a distinct type from U in the type system, but both have the same underlying storage type, and conversions between them preserves lvalueness (i.e., var x: T; U(x) = default(U) is valid)

According to this meaning, I'd say that the current cgen behaviour is correct. However, I'm not sure whether that's good and/or useful. In case it is, type T {.importc: N.} = distinct U should be always be an error, because a type cannot be imported (exact storage and target-language semantics are unknown) and at have the same time have the same storage type as some other type.

@saem
Copy link
Collaborator

saem commented Aug 14, 2024

According to this meaning, I'd say that the current cgen behaviour is correct. However, I'm not sure whether that's good and/or useful. In case it is, type T {.importc: N.} = distinct U should be always be an error, because a type cannot be imported (exact storage and target-language semantics are unknown) and at have the same time have the same storage type as some other type.

As I raised in matrix, my thinking is that it should be an error, because:

my present understanding is that importc is an escape hatch of sorts. you're basically saying "this thing is unrepresentable in skull" so it's kinda weird that we'd know anything/much about the guts of the type. so having much more than object, or possibly some other primitive type, seems odd.

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