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 behaviour for importc objects using importc objects internally #1419

Closed
shayanhabibi opened this issue Aug 15, 2024 · 2 comments
Closed
Labels
bug Something isn't working compiler/backend Related to backend system of the compiler duplicate This issue or pull request already exists

Comments

@shayanhabibi
Copy link
Collaborator

shayanhabibi commented Aug 15, 2024

Example

I have spent a bit of time debugging this today.
I admit this might be incorrect by our specifications, so I wanted to check and otherwise report it.

type
  int128 {.importc: "__int128".} = object
    `✀`, `✂` : uint64 # These fields are inaccessible
  hint128 = object
    x*,y*: uint

{.push, header: "<stdatomic.h>".}

type
  AtomicInt8* {.importc: "_Atomic NI8".} = int8
  AtomicInt16* {.importc: "_Atomic NI16".} = int16
  AtomicInt32* {.importc: "_Atomic NI32".} = int32
  AtomicInt64* {.importc: "_Atomic NI64".} = int64
  AtomicInt128* {.importc: "_Atomic __int128".} = int128 # INVALID compilation for use in 128bit atomicops

Please note the last line.
When performing generic procedures which would use this type, the codegen would generate a struct instead of an alias for type int128 {.importc: "__int128".} = object #....

If however it is changed to AtomicInt128* {.importc: "_Atomic __int128".} = hint128 then I do not have any issues. Is this intended? If so, perhaps causing such a type decl as an error would be helpful ~

type
  AtomicInt8* {.importc: "_Atomic NI8".} = int8
  AtomicInt16* {.importc: "_Atomic NI16".} = int16
  AtomicInt32* {.importc: "_Atomic NI32".} = int32
  AtomicInt64* {.importc: "_Atomic NI64".} = int64
  # AtomicInt128* {.importc: "_Atomic __int128".} = int128 # INVALID compilation for use in 128bit atomicops
  AtomicInt128*{.importc: "_Atomic __int128".} = hint128 # <- works

If this is borne from ignorance, please forgive me!

@shayanhabibi
Copy link
Collaborator Author

I have a small utility library based on this with tests that you can run to check the codegen.

Just change the line described in OP in the spec file to recreate the behaviour.

disclaimer: i run gcc on amd so my double word atomic ops run through __sync since they arent supported by __atomic on gcc

@zerbina
Copy link
Collaborator

zerbina commented Aug 15, 2024

A self-contained example that I believe reproduces the C compiler error reported here:

{.emit: "/*TYPESECTION*/ struct Imported {};\n".}
{.emit: "/*PROCSECTION*/ void native(struct Imported x) {}\n".}

type
  Imported {.importc: "struct Imported".} = object
  Alias {.importc: "struct Imported".} = Imported

proc native*(x: Imported) {.importc, nodecl.}

var y: Alias
var x: Imported
native(x)

The C compiler fails with:

error: incompatible type for argument 1 of 'native'
note: expected 'struct Imported' but argument is of type '_L8Imported_1_M6test52'

The underlying NimSkull bug is the same as the one causing #1417, which was fixed by #1418. Using the most recent compiler version, the test case from above compiles.

@zerbina zerbina added bug Something isn't working compiler/backend Related to backend system of the compiler labels Aug 15, 2024
@zerbina zerbina closed this as completed Aug 15, 2024
@saem saem added the duplicate This issue or pull request already exists label Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/backend Related to backend system of the compiler duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants