nim-works / nimskull

An in development statically typed systems programming language; with sustainability at its core. We, the community of users, maintain it.
https://nim-works.github.io/nimskull/index.html
Other
280 stars 39 forks source link

cgen incorrectly considers type to have C array semantics #1416

Open shayanhabibi opened 3 months ago

shayanhabibi commented 3 months ago

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 commented 3 months ago

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:

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 commented 3 months ago

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.