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

[WIP] fix {.incompleteStruct.} (implied wrong typeinfo) #14228

Closed

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented May 4, 2020

EDIT: DO NOT REVIEW YET...

the meaning of {.incompleteStruct.} is for types like DIR (eg http://manpages.ubuntu.com/manpages/bionic/man7/dirent.h.7posix.html) which are forward declared, and hence for which sizeof (or accessing members or creating an instance on the stack etc) would result in cgen CT error on some platforms. Treating it like we used to (via void*) is clearly wrong though. Investigating how to best handle this... stay tuned.

THE DESCRIPTION BELOW NEEDS TO BE UPDATED

needs a changelog entry and needs to be documented well

incompleteStruct (only used on 2 types, DIR and FILE) is problematic because when that pragma was set but the type was in fact not forward decared, it resulted in incorrect typeinfo

when true: # D20200504T161748
  type CFile {.importc: "FILE", header: "<stdio.h>", incompleteStruct.} = object
  let a = newSeq[CFile](1000)
  dumpNumberOfInstances()
(with -d:nimTypeNames)
before PR:
[Heap] seq[CFile]: #1; bytes: 8032
[Heap] total number of bytes: 8032
[Heap] allocs/deallocs: 6/0

after PR:
[Heap] seq[CFile]: #1; bytes: 152032
[Heap] total number of bytes: 152032
[Heap] allocs/deallocs: 6/0

note that FILE is a struct in C, not a struct pointer, so treating it as void* was incorrect as can be seen with this:

#include <stdio.h>
int main (int argc, char *argv[]) {
  printf("%d\n", sizeof(FILE));
  printf("%d\n", sizeof(FILE*));
  return 0;
}

links

When to use IncompleteStruct pragma? - Nim forum

@timotheecour timotheecour force-pushed the pr_remove_incompleteStruct branch from e9f095e to 6933f7e Compare May 4, 2020 23:23
@timotheecour timotheecour changed the title {.incompleteStruct.} is now a deprecated noop; was wrong default + implied wrong typeinfo [WIP] {.incompleteStruct.} is now a deprecated noop; was wrong default + implied wrong typeinfo May 5, 2020
@timotheecour timotheecour force-pushed the pr_remove_incompleteStruct branch from 965172b to 3db7539 Compare May 5, 2020 09:00
changelog.md Outdated Show resolved Hide resolved
compiler/pragmas.nim Outdated Show resolved Hide resolved
@timotheecour timotheecour force-pushed the pr_remove_incompleteStruct branch from 53d7fad to 32bacd0 Compare May 5, 2020 10:53
@timotheecour timotheecour force-pushed the pr_remove_incompleteStruct branch from 32bacd0 to 46ed26e Compare May 5, 2020 10:55
@timotheecour timotheecour marked this pull request as draft May 8, 2020 22:31
@timotheecour timotheecour changed the title [WIP] {.incompleteStruct.} is now a deprecated noop; was wrong default + implied wrong typeinfo [WIP] fix {.incompleteStruct.} (implied wrong typeinfo) Jul 4, 2020
@timotheecour timotheecour added the stale Staled PR/issues; remove the label after fixing them label Dec 1, 2020
@stale stale bot closed this Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants