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.

Sign up for GitHub

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

Jump to bottom

fix #14873 properly by skipping abi field in importc type #17944

Merged
merged 3 commits into from
May 7, 2021
Merged

fix #14873 properly by skipping abi field in importc type #17944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bec10d3
fix #14873 properly by skipping `abi` field in importc type
timotheecour May 6, 2021
dfc0fad
add test
timotheecour May 6, 2021
7678210
fix test for windows
timotheecour May 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions lib/core/locks.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,6 @@ type

{.push stackTrace: off.}


proc `$`*(lock: Lock): string =
# workaround bug #14873
result = "()"

proc initLock*(lock: var Lock) {.inline.} =
## Initializes the given lock.
when not defined(js):
Expand Down
8 changes: 0 additions & 8 deletions lib/system/syslocks.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,26 +102,18 @@ else:
SysLockObj {.importc: "pthread_mutex_t", pure, final,
header: """#include <sys/types.h>
#include <pthread.h>""".} = object
when defined(linux) and defined(amd64):
abi: array[40 div sizeof(clong), clong]

SysLockAttr {.importc: "pthread_mutexattr_t", pure, final
header: """#include <sys/types.h>
#include <pthread.h>""".} = object
when defined(linux) and defined(amd64):
abi: array[4 div sizeof(cint), cint] # actually a cint

SysCondObj {.importc: "pthread_cond_t", pure, final,
header: """#include <sys/types.h>
#include <pthread.h>""".} = object
when defined(linux) and defined(amd64):
abi: array[48 div sizeof(clonglong), clonglong]

SysCondAttr {.importc: "pthread_condattr_t", pure, final
header: """#include <sys/types.h>
#include <pthread.h>""".} = object
when defined(linux) and defined(amd64):
abi: array[4 div sizeof(cint), cint] # actually a cint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@timotheecour timotheecour May 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@timotheecour timotheecour May 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

@timotheecour timotheecour May 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


SysLockType = distinct cint

Expand Down
18 changes: 17 additions & 1 deletion tests/stdlib/uselocks.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,21 @@ proc use* (m: var MyType): int =
result = 3

block:
# bug #14873
var l: Lock
doAssert $l == "()"
doAssert ($l).len > 0
# on posix, "()", on windows, something else, but that shouldn't be part of the spec
# what matters is that `$` doesn't cause the codegen bug mentioned

when true: # intentional
# bug https://github.com/nim-lang/Nim/issues/14873#issuecomment-784241605
type
Test = object
path: string # Removing this makes both cases work.
lock: Lock
# A: This is not fine.
var a = Test()
proc main(): void =
# B: This is fine.
var b = Test()
main()