fix #14873 properly by skipping abi field in importc type

[timotheecour]

fix #14873

I can't reproduce #14873 locally, can someone that can reproduces those issues please check that it fails before this PR and works after this PR?

links:

• Codegen bug with lock - no member "abi" #14873 (comment)
• Codegen bug with lock - no member "abi" #14873 (comment)

/cc @shirleyquirk @xflywind @tdely

note

if this works, we should probably do the same with other apis, eg:

lib/posix/posix_linux_amd64.nim:153:5:    abi: array[56 div sizeof(clong), clong]
lib/posix/posix_linux_amd64.nim:157:5:    abi: array[32 div sizeof(clong), clong]
lib/posix/posix_linux_amd64.nim:160:5:    abi: array[4 div sizeof(cint), cint]
lib/posix/posix_linux_amd64.nim:164:5:    abi: array[48 div sizeof(clonglong), clonglong]
lib/posix/posix_linux_amd64.nim:167:5:    abi: array[4 div sizeof(cint), cint]
lib/posix/posix_linux_amd64.nim:171:5:    abi: array[48 div sizeof(clong), clong]
lib/posix/posix_linux_amd64.nim:174:5:    abi: array[4 div sizeof(cint), cint]
lib/posix/posix_linux_amd64.nim:178:5:    abi: array[56 div sizeof(clong), clong]
lib/posix/posix_linux_amd64.nim:181:5:    abi: array[8 div sizeof(clong), clong]
lib/posix/posix_linux_amd64.nim:204:5:    abi: array[32 div sizeof(clong), clong]
lib/posix/posix_linux_amd64.nim:289:5:    abi: array[1024 div (8 * sizeof(culong)), culong]
lib/posix/posix_linux_amd64.nim:298:5:    abi: array[12, int]
lib/posix/posix_linux_amd64.nim:353:5:    abi: array[1024 div (8 * sizeof(clong)), clong]
lib/posix/posix_nintendoswitch.nim:132:5:    abi: array[56 div sizeof(clong), clong]
lib/posix/posix_nintendoswitch.nim:136:5:    abi: array[32 div sizeof(clong), clong]
lib/posix/posix_nintendoswitch.nim:139:5:    abi: array[4 div sizeof(cint), cint]
lib/posix/posix_nintendoswitch.nim:143:5:    abi: array[48 div sizeof(clonglong), clonglong]
lib/posix/posix_nintendoswitch.nim:146:5:    abi: array[4 div sizeof(cint), cint]
lib/posix/posix_nintendoswitch.nim:150:5:    abi: array[48 div sizeof(clong), clong]
lib/posix/posix_nintendoswitch.nim:153:5:    abi: array[4 div sizeof(cint), cint]
lib/posix/posix_nintendoswitch.nim:157:5:    abi: array[56 div sizeof(clong), clong]
lib/posix/posix_nintendoswitch.nim:160:5:    abi: array[8 div sizeof(clong), clong]
lib/posix/posix_nintendoswitch.nim:183:5:    abi: array[32 div sizeof(clong), clong]
lib/posix/posix_nintendoswitch.nim:310:5:    abi: array[((64+(sizeof(clong) * 8)-1) div (sizeof(clong) * 8)), clong]
lib/system/ansi_c.nim:37:9:        abi: array[200 div sizeof(clong), clong]
lib/system/threadlocalstorage.nim:135:9:        abi: array[56 div sizeof(clong), clong]
lib/system/threadlocalstorage.nim:194:8:       abi: array[1024 div (8 * sizeof(culong)), culong]

[Araq]

Changes like these break nlvm (which is still our best idea of how to get good debugging support btw).

[timotheecour]

but why single out this type? other types have no guarantee to have all their fields listed in the importc type (unless {.completeStruct.} is given), nor being in order

furthermore, nlvm is heavily behind nim devel: last nim commit that was integrated is Jul 29, 2020, arnetheduck@bf320ed)

and there are other ways to do this without resorting to platform specific conditional fields.

abi/padding fields that aren't used just shouldn't be exposed, which leads to bugs like #14873 etc

more robust ways to query for fields for an importc type

we could query the type using libclang / cling (timotheecour#705), leaving guesswork out of the equation. This could also be done optionally, leaving out uncertain fields by default (as done in this PR)

this approach is actually useful for many problems, including all the importc let variables that are currently hardcoded but could instead be queried (see also nim-lang/RFCs#205). It's not hard to do at least on posix.

[alaviss]

Don't we have a size pragma? Can it be used as a replacement for abi?

[timotheecour]

yes, that would be better than abi field, but it still causes the same problems because the ABI for those is not specified and varies by OS/distribution.

note that you can still use sizeof at RT; {.sizeof: N.} only makes sense if you need it at CT (unlikely because, well, VM is single threaded).

Note also that on osx i get:

import std/locks
var a: Lock
echo a.sizeof # 64

even though no nim abi field is there in the definition of SysLockObj, which shows how arbitrary this was. Furthermore, it's also wrong for other architectures besides osx (eg: linux and not amd64)

Long story short: removing the abi, as i did in this PR, makes sense.

links

• https://stackoverflow.com/a/30585442/1426932

You'll want to use it as their defined type, pthread_mutex_t of course -- since this type will vary by OS / distribution / etc.

[timotheecour]

@Araq also you can always do this if you need to access raw bytes:

import std/[sugar,locks]

proc getBytes[T](buf: var seq[uint8], a: T) =
  let n = T.sizeof
  buf.setLen n
  copyMem(buf[0].addr, a.unsafeAddr, n)

var a: Lock
initLock(a)

echo seq[uint8].default.dup(getBytes(a)) # @[90, 84, 85, 77, 0, 0, 0, 0, ... 85, 77, 90, 84, 85, 77]

unlike abi field which is fragile and not portable (eg doesn't work on osx etc), this will always work

=> timotheecour#722

[Araq]

The abi field never was about accessing bytes, it was about getting sizeof and alignof right.

[timotheecour]

ok; but sizeof and alignof don't need abi fields so long sizeof and alignof are only accessed at RT:

# after this PR is merged (and on osx) I'm getting:
import std/locks
var a: Lock
echo a.sizeof # 64
echo a.alignof # 8

but I think we're agreeing at this point

[arnetheduck]

need it at CT

you need it for pointer arithmetic, memory allocation, offset arithmetic and a number of other cases - this PR breaks existing code and introduces differences between compile time and runtime that lead to bugs.

The proper fix is to provide the Nim compiler with correct information at compile time - this can indeed be through libclang - or by just spelling it out.

[arnetheduck]

nlvm is heavily behind nim devel

nlvm is behind mainly because breaking things and moving fast doesn't work for code that actually depends on the standard library to not break their code, ie actual users of the language and the library - it's PR's like this that prevent it from being updated, as well as drive users of it to look for alternative solutions, including forks - the bar of quality must be higher than "works for me, don't care about the rest" - as long as this is not understood (as can be seen by similar PR's recently), it's a hard sell to actually use the code and the end result is a cycle of "I'll make it work for my case now" style back-and-forth PRs. It's not a sustainable approach.

[Araq]

Your arguments were convincing.

[arnetheduck]

The ABI fields were added because they make certain code work that previously didn't, particularily such code that is careful about memory allocation - this PR breaks such code, and should ideally be rolled back.