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

changelog.md

@@ -120,12 +120,18 @@
   avoid recompiling when sources don't change. This is now the preferred way to
   run tests, avoiding the usual pain of clobbering your repo with binaries or
   using tricky gitignore rules on posix. Example:
-  ```nim
+  ```bash
   nim r compiler/nim.nim --help # only compiled the first time
-  echo 'import os; echo getCurrentCompilerExe()' | nim r - # this works too
+  echo "import os; echo getCurrentCompilerExe()" | nim r - # this works too
   nim r compiler/nim.nim --fullhelp # no recompilation
   nim r --nimcache:/tmp main # binary saved to /tmp/main
   ```
+- a new pragma `completeStruct` for importc objects indicates the type is fully
+  specified in the nim declaration (including field ordering), and allows
+  `sizeof, alignof, offsetof` to be used at compile time, see
+  [manual](manual.html#implementation-specific-pragmas-completestruct-pragma).
+- `incompleteStruct` is now deprecated, see
+  [manual](manual.html#implementation-specific-pragmas-incompletestruct-pragma).
 
 ## Tool changes
 

compiler/ast.nim

@@ -552,7 +552,6 @@ type
     tfCheckedForDestructor # type was checked for having a destructor.
                            # If it has one, t.destructor is not nil.
     tfAcyclic # object type was annotated as .acyclic
-    tfIncompleteStruct # treat this type as if it had sizeof(pointer)
     tfCompleteStruct
       # (for importc types); type is fully specified, allowing to compute
       # sizeof, alignof, offsetof at CT

compiler/ccgtypes.nim

@@ -888,8 +888,6 @@ proc getTypeDescAux(m: BModule, origTyp: PType, check: var IntSet): Rope =
                       else: getTupleDesc(m, t, result, check)
         if not isImportedType(t):
           m.s[cfsTypes].add(recdesc)
-        elif tfIncompleteStruct notin t.flags:
-          discard # addAbiCheck(m, t, result) # already handled elsewhere
   of tySet:
     # Don't use the imported name as it may be scoped: 'Foo::SomeKind'
     result = $t.kind & '_' & t.lastSon.typeName & $t.lastSon.hashType
@@ -1008,14 +1006,9 @@ proc genTypeInfoAuxBase(m: BModule; typ, origType: PType;
 
   let nameHcr = tiNameForHcr(m, name)
 
-  var size: Rope
-  if tfIncompleteStruct in typ.flags:
-    size = rope"void*"
-  else:
-    size = getTypeDesc(m, origType)
   m.s[cfsTypeInit3].addf(
     "$1.size = sizeof($2);$n$1.align = NIM_ALIGNOF($2);$n$1.kind = $3;$n$1.base = $4;$n",
-    [nameHcr, size, rope(nimtypeKind), base]
+    [nameHcr, getTypeDesc(m, origType), rope(nimtypeKind), base]
   )
   # compute type flags for GC optimization
   var flags = 0

compiler/pragmas.nim

@@ -765,6 +765,10 @@ proc semCustomPragma(c: PContext, n: PNode): PNode =
 proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int,
                   validPragmas: TSpecialWords,
                   comesFromPush, isStatement: bool): bool =
+
+  template deprecatedPragma(keyword, msg) =
+    localError(c.config, n.info, warnDeprecated, "pragma '$1' is deprecated: $2" % [keyword.canonPragmaSpelling, msg])
+
   var it = n[i]
   var key = if it.kind in nkPragmaCallKinds and it.len > 1: it[0] else: it
   if key.kind == nkBracketExpr:
@@ -867,7 +871,8 @@ proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int,
       of wThreadVar:
         noVal(c, it)
         incl(sym.flags, {sfThread, sfGlobal})
-      of wDeadCodeElimUnused: discard  # deprecated, dead code elim always on
+      of wDeadCodeElimUnused: deprecatedPragma(wDeadCodeElimUnused, "dead code elim always on")
+      of wIncompleteStruct: deprecatedPragma(wIncompleteStruct, "see https://nim-lang.github.io/Nim/manual.html#implementation-specific-pragmas-incompletestruct-pragma")
       of wNoForward: pragmaNoForward(c, it)
       of wReorder: pragmaNoForward(c, it, flag = sfReorder)
       of wMagic: processMagic(c, it, sym)
@@ -1068,10 +1073,6 @@ proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int,
       of wEffects:
         # is later processed in effect analysis:
         noVal(c, it)
-      of wIncompleteStruct:
-        noVal(c, it)
-        if sym.typ == nil: invalidPragma(c, it)
-        else: incl(sym.typ.flags, tfIncompleteStruct)
       of wCompleteStruct:
         noVal(c, it)
         if sym.typ == nil: invalidPragma(c, it)

compiler/wordrecg.nim

@@ -41,7 +41,7 @@ type
     wImportCpp, wImportObjC,
     wImportCompilerProc,
     wImportc, wImportJs, wExportc, wExportCpp, wExportNims,
-    wIncompleteStruct, # deprecated
+    wIncompleteStruct, # deprecated since 1.3.1 now that we have wCompleteStruct
     wCompleteStruct,
     wRequiresInit,
     wAlign, wNodecl, wPure, wSideEffect, wHeader,

doc/manual.rst

@@ -6408,17 +6408,42 @@ encloses the header file in ``""`` in the generated C code.
 **Note**: This will not work for the LLVM backend.
 
 
-IncompleteStruct pragma
+CompleteStruct pragma
 -----------------------
-The ``incompleteStruct`` pragma tells the compiler to not use the
-underlying C ``struct`` in a ``sizeof`` expression:
+By default, `importc` types are considered incomplete: they may be lacking fields
+in their nim declaration because these are unused in nim code or because of ABI
+differences across platforms, in particular for padding fields. Field reorderings
+are also allowed. This prevents using `sizeof`, `alignof`, `offsetOf` at compile
+time.
+These can still be used at runtime, deferring to calling `sizeof` (+ friends) in the
+backend generated code. In this case, they're known at backend compile time,
+but not during nim semantic phase.
+
+This `completeStruct` pragma overrides this behavior by telling the compiler
+the type declaration in nim is fully specified and in correct order so that
+`sizeof`, `alignof`, `offsetOf` are available at compile time.
+As a sanity check, `-d:checkAbi` will insert backend static checks to make sure
+at least `sizeof` is correct.
 
 .. code-block:: Nim
   type
-    DIR* {.importc: "DIR", header: "<dirent.h>",
-           pure, incompleteStruct.} = object
+    TFoo* {.importc: "struct Foo", header: "<bar.h>", completeStruct.} = object
+      s1*: cint
+      x2* {.importc: "_x2"}: pointer # renamed field
+
+See `additional examples <https://github.com/nim-lang/Nim/blob/devel/tests/misc/msizeof5.nim>`_.
 
 
+IncompleteStruct pragma
+-----------------------
+**deprecated**
+This was used for forwarded C declarations such as `DIR` on some platforms,
+but led to incorrect typeinfo (eg with `dumpNumberOfInstances`). Instead we
+simply avoid referring to such type by value on platforms where it's a forwarded
+declaration and only use it via pointer. Any disallowed use-by-value leads to a
+C codegen error, and we avoid silently wrong results for cases where
+use-by-value is allowed.
+
 Compile pragma
 --------------
 The ``compile`` pragma can be used to compile and link a C/C++ source file

lib/posix/posix.nim

@@ -95,6 +95,20 @@ const StatHasNanoseconds* = defined(linux) or defined(freebsd) or
   ## without checking this flag, because this module defines fallback procs
   ## when they are not available.
 
+# Platform common stuff (avoids repetition)
+
+type
+  DIR {.importc: "DIR", header: "<dirent.h>".} = object
+    ## on some systems, this is a forward declared type and should only
+    ## be used via pointer, ie via `C_DIR`
+  C_DIR*  = ptr DIR
+    ## A ptr type representing a directory stream.
+
+when defined(nimBackendHasPosixDIR):
+  # This should be auto-detected, like NIM_EmulateOverflowChecks, by
+  # calling preprocessor, see https://github.com/nim-lang/RFCs/issues/205 prop 5
+  export DIR
+
 # Platform specific stuff
 
 when (defined(linux) and not defined(android)) and defined(amd64):
@@ -166,14 +180,14 @@ proc IN6ADDR_ANY_INIT* (): In6Addr {.importc, header: "<netinet/in.h>".}
 proc IN6ADDR_LOOPBACK_INIT* (): In6Addr {.importc, header: "<netinet/in.h>".}
 
 # dirent.h
-proc closedir*(a1: ptr DIR): cint  {.importc, header: "<dirent.h>".}
-proc opendir*(a1: cstring): ptr DIR {.importc, header: "<dirent.h>", sideEffect.}
-proc readdir*(a1: ptr DIR): ptr Dirent  {.importc, header: "<dirent.h>", sideEffect.}
-proc readdir_r*(a1: ptr DIR, a2: ptr Dirent, a3: ptr ptr Dirent): cint  {.
+proc closedir*(a1: C_DIR): cint  {.importc, header: "<dirent.h>".}
+proc opendir*(a1: cstring): C_DIR {.importc, header: "<dirent.h>", sideEffect.}
+proc readdir*(a1: C_DIR): ptr Dirent  {.importc, header: "<dirent.h>", sideEffect.}
+proc readdir_r*(a1: C_DIR, a2: ptr Dirent, a3: ptr ptr Dirent): cint  {.
                 importc, header: "<dirent.h>", sideEffect.}
-proc rewinddir*(a1: ptr DIR)  {.importc, header: "<dirent.h>".}
-proc seekdir*(a1: ptr DIR, a2: int)  {.importc, header: "<dirent.h>".}
-proc telldir*(a1: ptr DIR): int {.importc, header: "<dirent.h>".}
+proc rewinddir*(a1: C_DIR)  {.importc, header: "<dirent.h>".}
+proc seekdir*(a1: C_DIR, a2: int)  {.importc, header: "<dirent.h>".}
+proc telldir*(a1: C_DIR): int {.importc, header: "<dirent.h>".}
 
 # dlfcn.h
 proc dlclose*(a1: pointer): cint {.importc, header: "<dlfcn.h>", sideEffect.}

lib/posix/posix_haiku.nim

@@ -23,11 +23,6 @@ when defined(solaris):
   # On Solaris hstrerror lives in libresolv
   {.passl: "-lresolv".}
 
-type
-  DIR* {.importc: "DIR", header: "<dirent.h>",
-          incompleteStruct.} = object
-    ## A type representing a directory stream.
-
 type
   SocketHandle* = distinct cint # The type used to represent socket descriptors
 

lib/posix/posix_linux_amd64.nim

@@ -26,11 +26,6 @@ when defined(nimHasStyleChecks):
 
 # Types
 
-type
-  DIR* {.importc: "DIR", header: "<dirent.h>",
-          incompleteStruct.} = object
-    ## A type representing a directory stream.
-
 type
   SocketHandle* = distinct cint # The type used to represent socket descriptors
 

lib/posix/posix_macos_amd64.nim

@@ -14,11 +14,6 @@ const
   hasSpawnH = true # should exist for every Posix system nowadays
   hasAioH = false
 
-type
-  DIR* {.importc: "DIR", header: "<dirent.h>",
-          incompleteStruct.} = object
-    ## A type representing a directory stream.
-
 type
   SocketHandle* = distinct cint # The type used to represent socket descriptors
 

lib/posix/posix_nintendoswitch.nim

@@ -13,10 +13,6 @@ const
   hasSpawnH = true
   hasAioH = false
 
-type
-  DIR* {.importc: "DIR", header: "<dirent.h>",
-          incompleteStruct.} = object
-
 const SIG_HOLD* = cast[Sighandler](2)
 
 type

lib/posix/posix_openbsd_amd64.nim

@@ -14,11 +14,6 @@ const
   hasSpawnH = true # should exist for every Posix system nowadays
   hasAioH = false
 
-type
-  DIR* {.importc: "DIR", header: "<dirent.h>",
-          incompleteStruct.} = object
-    ## A type representing a directory stream.
-
 type
   SocketHandle* = distinct cint # The type used to represent socket descriptors
 

lib/posix/posix_other.nim

@@ -23,11 +23,6 @@ when defined(solaris):
   # On Solaris hstrerror lives in libresolv
   {.passl: "-lresolv".}
 
-type
-  DIR* {.importc: "DIR", header: "<dirent.h>",
-          incompleteStruct.} = object
-    ## A type representing a directory stream.
-
 type
   SocketHandle* = distinct cint # The type used to represent socket descriptors
 

lib/system/ansi_c.nim

@@ -112,8 +112,7 @@ proc c_signal*(sign: cint, handler: proc (a: cint) {.noconv.}): CSighandlerT {.
   importc: "signal", header: "<signal.h>", discardable.}
 
 type
-  CFile {.importc: "FILE", header: "<stdio.h>",
-          incompleteStruct.} = object
+  CFile {.importc: "FILE", header: "<stdio.h>".} = object
   CFilePtr* = ptr CFile ## The type representing a file handle.
 
 # duplicated between io and ansi_c

lib/system/io.nim

@@ -16,8 +16,7 @@ import formatfloat
 
 # ----------------- IO Part ------------------------------------------------
 type
-  CFile {.importc: "FILE", header: "<stdio.h>",
-          incompleteStruct.} = object
+  CFile {.importc: "FILE", header: "<stdio.h>".} = object
   File* = ptr CFile ## The type representing a file handle.
 
   FileMode* = enum           ## The file mode when opening a file.