CT sizeof(+friends) for {.importc, completeStruct.} types, enable ABI static checks (#13926)

* -d:checkabi obsolete (ABI check now enforced); add `addTypeHeader` helper
* cleanups
* import sizeof at CT for {.completeType.}
* address comments; revert default enabling of -d:checkAbi for now
* mimportc_size_check.nim => msizeof5.nim; merge mabi_check.nim into msizeof5.nim; refactor
* all pragmas in errmsgs should be written: '.importc' (un-ambiguous and less verbose than {.importc.})

compiler/ast.nim

@@ -553,6 +553,9 @@ type
                            # 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
 
   TTypeFlags* = set[TTypeFlag]
 

compiler/ccgmerge.nim

@@ -48,6 +48,10 @@ const
   NimMergeEndMark = "/*\tNIM_merge_END:*/"
 
 proc genSectionStart*(fs: TCFileSection; conf: ConfigRef): Rope =
+  # useful for debugging and only adds at most a few lines in each file
+  result.add("\n/* section: ")
+  result.add(CFileSectionNames[fs])
+  result.add(" */\n")
   if compilationCachePresent(conf):
     result = nil
     result.add("\n/*\t")

compiler/ccgtypes.nim

@@ -247,8 +247,13 @@ proc cacheGetType(tab: TypeCache; sig: SigHash): Rope =
   result = tab.getOrDefault(sig)
 
 proc addAbiCheck(m: BModule, t: PType, name: Rope) =
-  if isDefined(m.config, "checkabi") and (let size = getSize(m.config, t); size != szUnknownSize):
-    m.s[cfsTypeInfo].addf("NIM_CHECK_SIZE($1, $2);$n", [name, rope(size)])
+  if isDefined(m.config, "checkAbi") and (let size = getSize(m.config, t); size != szUnknownSize):
+    var msg = "backend & Nim disagree on size for: "
+    msg.addTypeHeader(m.config, t)
+    var msg2 = ""
+    msg2.addQuoted msg # not a hostspot so extra allocation doesn't matter
+    m.s[cfsTypeInfo].addf("NIM_STATIC_ASSERT(sizeof($1) == $2, $3);$n", [name, rope(size), msg2.rope])
+    # see `testCodegenABICheck` for example error message it generates
 
 proc ccgIntroducedPtr(conf: ConfigRef; s: PSym, retType: PType): bool =
   var pt = skipTypes(s.typ, typedescInst)
@@ -328,7 +333,6 @@ proc getSimpleTypeDesc(m: BModule, typ: PType): Rope =
     let sig = hashType typ
     if cacheGetType(m.typeCache, sig) == nil:
       m.typeCache[sig] = result
-      addAbiCheck(m, typ, result)
 
 proc pushType(m: BModule, typ: PType) =
   for i in 0..high(m.typeStack):
@@ -676,6 +680,7 @@ proc resolveStarsInCppType(typ: PType, idx, stars: int): PType =
 
 proc getTypeDescAux(m: BModule, origTyp: PType, check: var IntSet): Rope =
   # returns only the type's name
+
   var t = origTyp.skipTypes(irrelevantForBackend-{tyOwned})
   if containsOrIncl(check, t.id):
     if not (isImportedCppType(origTyp) or isImportedCppType(t)):
@@ -686,6 +691,11 @@ proc getTypeDescAux(m: BModule, origTyp: PType, check: var IntSet): Rope =
   if t.sym != nil: useHeader(m, t.sym)
   if t != origTyp and origTyp.sym != nil: useHeader(m, origTyp.sym)
   let sig = hashType(origTyp)
+
+  defer: # defer is the simplest in this case
+    if isImportedType(t) and not m.typeABICache.containsOrIncl(sig):
+      addAbiCheck(m, t, result)
+
   result = getTypePre(m, t, sig)
   if result != nil:
     excl(check, t.id)
@@ -818,7 +828,6 @@ proc getTypeDescAux(m: BModule, origTyp: PType, check: var IntSet): Rope =
       let foo = getTypeDescAux(m, t[1], check)
       m.s[cfsTypes].addf("typedef $1 $2[$3];$n",
            [foo, result, rope(n)])
-    else: addAbiCheck(m, t, result)
   of tyObject, tyTuple:
     if isImportedCppType(t) and origTyp.kind == tyGenericInst:
       let cppName = getTypeName(m, t, sig)
@@ -879,7 +888,8 @@ 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: addAbiCheck(m, t, result)
+        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

compiler/cgendata.nim

@@ -11,7 +11,7 @@
 
 import
   ast, ropes, options, intsets,
-  tables, ndi, lineinfos, pathutils, modulegraphs
+  tables, ndi, lineinfos, pathutils, modulegraphs, sets
 
 type
   TLabel* = Rope              # for the C generator a label is just a rope
@@ -25,7 +25,7 @@ type
                               # this is needed for strange type generation
                               # reasons
     cfsFieldInfo,             # section for field information
-    cfsTypeInfo,              # section for type information
+    cfsTypeInfo,              # section for type information (ag ABI checks)
     cfsProcHeaders,           # section for C procs prototypes
     cfsData,                  # section for C constant data
     cfsVars,                  # section for C variable declarations
@@ -144,6 +144,10 @@ type
                               # without extension)
     tmpBase*: Rope            # base for temp identifier generation
     typeCache*: TypeCache     # cache the generated types
+    typeABICache*: HashSet[SigHash] # cache for ABI checks; reusing typeCache
+                              # would be ideal but for some reason enums
+                              # don't seem to get cached so it'd generate
+                              # 1 ABI check per occurence in code
     forwTypeCache*: TypeCache # cache for forward declarations of types
     declaredThings*: IntSet   # things we have declared in this .c file
     declaredProtos*: IntSet   # prototypes we have declared in this .c file

compiler/pragmas.nim

@@ -61,7 +61,7 @@ const
     wGcSafe, wCodegenDecl} - {wExportNims, wError, wUsed}  # why exclude these?
   typePragmas* = declPragmas + {wMagic, wAcyclic,
     wPure, wHeader, wCompilerProc, wCore, wFinal, wSize, wShallow,
-    wIncompleteStruct, wByCopy, wByRef,
+    wIncompleteStruct, wCompleteStruct, wByCopy, wByRef,
     wInheritable, wGensym, wInject, wRequiresInit, wUnchecked, wUnion, wPacked,
     wBorrow, wGcSafe, wPartial, wExplain, wPackage}
   fieldPragmas* = declPragmas + {
@@ -1072,6 +1072,10 @@ proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int,
         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)
+        else: incl(sym.typ.flags, tfCompleteStruct)
       of wUnchecked:
         noVal(c, it)
         if sym.typ == nil or sym.typ.kind notin {tyArray, tyUncheckedArray}:

compiler/sizealignoffsetimpl.nim

@@ -382,7 +382,8 @@ proc computeSizeAlign(conf: ConfigRef; typ: PType) =
         computeObjectOffsetsFoldFunction(conf, typ.n, false, accum)
       let paddingAtEnd = int16(accum.finish())
       if typ.sym != nil and
-         typ.sym.flags * {sfCompilerProc, sfImportc} == {sfImportc}:
+         typ.sym.flags * {sfCompilerProc, sfImportc} == {sfImportc} and
+         tfCompleteStruct notin typ.flags:
         typ.size = szUnknownSize
         typ.align = szUnknownSize
         typ.paddingAtEnd = szUnknownSize

compiler/types.nim

@@ -22,7 +22,9 @@ type
     preferGenericArg,
     preferTypeName,
     preferResolved, # fully resolved symbols
-    preferMixed, # show symbol + resolved symbols if it differs, eg: seq[cint{int32}, float]
+    preferMixed,
+      # most useful, shows: symbol + resolved symbols if it differs, eg:
+      # tuple[a: MyInt{int}, b: float]
 
 proc typeToString*(typ: PType; prefer: TPreferedDesc = preferName): string
 template `$`*(typ: PType): string = typeToString(typ)
@@ -121,6 +123,13 @@ proc isIntLit*(t: PType): bool {.inline.} =
 proc isFloatLit*(t: PType): bool {.inline.} =
   result = t.kind == tyFloat and t.n != nil and t.n.kind == nkFloatLit
 
+proc addDeclaredLoc(result: var string, conf: ConfigRef; sym: PSym) =
+  result.add " [declared in " & conf$sym.info & "]"
+
+proc addTypeHeader*(result: var string, conf: ConfigRef; typ: PType; prefer: TPreferedDesc = preferMixed; getDeclarationPath = true) =
+  result.add typeToString(typ, prefer)
+  if getDeclarationPath: result.addDeclaredLoc(conf, typ.sym)
+
 proc getProcHeader*(conf: ConfigRef; sym: PSym; prefer: TPreferedDesc = preferName; getDeclarationPath = true): string =
   assert sym != nil
   # consider using `skipGenericOwner` to avoid fun2.fun2 when fun2 is generic
@@ -140,10 +149,7 @@ proc getProcHeader*(conf: ConfigRef; sym: PSym; prefer: TPreferedDesc = preferNa
     result.add(')')
     if n[0].typ != nil:
       result.add(": " & typeToString(n[0].typ, prefer))
-  if getDeclarationPath:
-    result.add " [declared in "
-    result.add(conf$sym.info)
-    result.add "]"
+  if getDeclarationPath: result.addDeclaredLoc(conf, sym)
 
 proc elemType*(t: PType): PType =
   assert(t != nil)

compiler/vmgen.nim

@@ -726,6 +726,9 @@ proc genBinaryABCD(c: PCtx; n: PNode; dest: var TDest; opc: TOpcode) =
   c.freeTemp(tmp2)
   c.freeTemp(tmp3)
 
+template sizeOfLikeMsg(name): string =
+  "'$1' requires '.importc' types to be '.completeStruct'" % [name]
+
 proc genNarrow(c: PCtx; n: PNode; dest: TDest) =
   let t = skipTypes(n.typ, abstractVar-{tyTypeDesc})
   # uint is uint64 in the VM, we we only need to mask the result for
@@ -1317,11 +1320,11 @@ proc genMagic(c: PCtx; n: PNode; dest: var TDest; m: TMagic) =
     else:
       globalError(c.config, n.info, "expandToAst requires a call expression")
   of mSizeOf:
-    globalError(c.config, n.info, "cannot evaluate 'sizeof' because its type is not defined completely")
+    globalError(c.config, n.info, sizeOfLikeMsg("sizeof"))
   of mAlignOf:
-    globalError(c.config, n.info, "cannot evaluate 'alignof' because its type is not defined completely")
+    globalError(c.config, n.info, sizeOfLikeMsg("alignof"))
   of mOffsetOf:
-    globalError(c.config, n.info, "cannot evaluate 'offsetof' because its type is not defined completely")
+    globalError(c.config, n.info, sizeOfLikeMsg("offsetof"))
   of mRunnableExamples:
     discard "just ignore any call to runnableExamples"
   of mDestroy: discard "ignore calls to the default destructor"

compiler/wordrecg.nim

@@ -40,7 +40,10 @@ type
     wImmediate, wConstructor, wDestructor, wDelegator, wOverride,
     wImportCpp, wImportObjC,
     wImportCompilerProc,
-    wImportc, wImportJs, wExportc, wExportCpp, wExportNims, wIncompleteStruct, wRequiresInit,
+    wImportc, wImportJs, wExportc, wExportCpp, wExportNims,
+    wIncompleteStruct, # deprecated
+    wCompleteStruct,
+    wRequiresInit,
     wAlign, wNodecl, wPure, wSideEffect, wHeader,
     wNoSideEffect, wGcSafe, wNoreturn, wNosinks, wMerge, wLib, wDynlib,
     wCompilerProc, wCore, wProcVar, wBase, wUsed,
@@ -128,6 +131,7 @@ const
     "importcpp", "importobjc",
     "importcompilerproc", "importc", "importjs", "exportc", "exportcpp", "exportnims",
     "incompletestruct",
+    "completestruct",
     "requiresinit", "align", "nodecl", "pure", "sideeffect",
     "header", "nosideeffect", "gcsafe", "noreturn", "nosinks", "merge", "lib", "dynlib",
     "compilerproc", "core", "procvar", "base", "used",
@@ -186,6 +190,7 @@ proc canonPragmaSpelling*(w: TSpecialWord): string =
   of wNoSideEffect: "noSideEffect"
   of wImportCompilerProc: "importCompilerProc"
   of wIncompleteStruct: "incompleteStruct"
+  of wCompleteStruct: "completeStruct"
   of wRequiresInit: "requiresInit"
   of wSideEffect: "sideEffect"
   of wLineDir: "lineDir"

doc/nimc.rst

@@ -438,9 +438,8 @@ Define                   Effect
 ``memProfiler``          Enables memory profiling for the native GC.
 ``uClibc``               Use uClibc instead of libc. (Relevant for Unix-like OSes)
 ``checkAbi``             When using types from C headers, add checks that compare
-                         what's in the Nim file with what's in the C header
-                         (requires a C compiler with _Static_assert support, like
-                         any C11 compiler)
+                         what's in the Nim file with what's in the C header.
+                         This may become enabled by default in the future.
 ``tempDir``              This symbol takes a string as its value, like
                          ``--define:tempDir:/some/temp/path`` to override the
                          temporary directory returned by ``os.getTempDir()``.

lib/nimbase.h

@@ -562,10 +562,6 @@ NIM_STATIC_ASSERT(sizeof(NI) == sizeof(void*) && NIM_INTBITS == sizeof(NI)*8, ""
 #  include <sys/types.h>
 #endif
 
-/* Compile with -d:checkAbi and a sufficiently C11:ish compiler to enable */
-#define NIM_CHECK_SIZE(typ, sz) \
-  _Static_assert(sizeof(typ) == sz, "Nim & C disagree on type size")
-
 /* these exist to make the codegen logic simpler */
 #define nimModInt(a, b, res) (((*res) = (a) % (b)), 0)
 #define nimModInt64(a, b, res) (((*res) = (a) % (b)), 0)

tests/misc/msizeof5.nim

@@ -0,0 +1,131 @@
+## tests for -d:checkAbi used by addAbiCheck via NIM_STATIC_ASSERT
+
+{.emit:"""/*TYPESECTION*/
+struct Foo1{
+  int a;
+};
+struct Foo2{
+  int a;
+};
+enum Foo3{k1, k2};
+typedef enum Foo3 Foo3b;
+typedef enum Foo4{k3, k4} Foo4;
+
+typedef int Foo5[3];
+
+typedef struct Foo6{
+  int a1;
+  bool a2;
+  double a3;
+  struct Foo6* a4;
+} Foo6;
+""".}
+
+template ensureCgen(T: typedesc) =
+  ## ensures cgen
+  var a {.volatile.}: T
+
+block:
+  type Foo1Alias{.importc: "struct Foo1", size: sizeof(cint).} = object
+    a: cint
+  ensureCgen Foo1Alias
+
+block:
+  type Foo3Alias{.importc: "enum Foo3", size: sizeof(cint).} = enum
+    k1, k2
+  ensureCgen Foo3Alias
+
+block:
+  type Foo3bAlias{.importc: "Foo3b", size: sizeof(cint).} = enum
+    k1, k2
+  ensureCgen Foo3bAlias
+
+block:
+  type Foo3b{.importc, size: sizeof(cint).} = enum
+    k1, k2
+  ensureCgen Foo3b
+  static:
+    doAssert Foo3b.sizeof == cint.sizeof
+
+block:
+  type Foo4{.importc, size: sizeof(cint).} = enum
+    k3, k4
+  # adding entries should not yield duplicate ABI checks, as enforced by
+  # `typeABICache`.
+  # Currently the test doesn't check for this but you can inspect the cgen'd file
+  ensureCgen Foo4
+  ensureCgen Foo4
+  ensureCgen Foo4
+
+block:
+  type Foo5{.importc.} = array[3, cint]
+  ensureCgen Foo5
+
+block:
+  type Foo5{.importc.} = array[3, cint]
+  ensureCgen Foo5
+
+block: # CT sizeof
+  type Foo6GT = object # grountruth
+    a1: cint
+    a2: bool
+    a3: cfloat
+    a4: ptr Foo6GT
+
+  type Foo6{.importc, completeStruct.} = object
+    a1: cint
+    a2: bool
+    a3: cfloat
+    a4: ptr Foo6
+
+  static: doAssert compiles(static(Foo6.sizeof))
+  static: doAssert Foo6.sizeof == Foo6GT.sizeof
+  static: doAssert (Foo6, int, array[2, Foo6]).sizeof ==
+    (Foo6GT, int, array[2, Foo6GT]).sizeof
+
+block:
+  type GoodImportcType {.importc: "signed char", nodecl.} = char
+    # "good" in sense the sizeof will match
+  ensureCgen GoodImportcType
+
+block:
+  type Foo6{.importc.} = object
+    a1: cint
+  doAssert compiles(Foo6.sizeof)
+  static: doAssert not compiles(static(Foo6.sizeof))
+
+when defined caseBad:
+  # Each case below should give a static cgen assert fail message
+
+  block:
+    type BadImportcType {.importc: "unsigned char", nodecl.} = uint64
+      # "sizeof" check will fail
+    ensureCgen BadImportcType
+
+  block:
+    type Foo2AliasBad{.importc: "struct Foo2", size: 1.} = object
+      a: cint
+    ensureCgen Foo2AliasBad
+
+  block:
+    type Foo5{.importc.} = array[4, cint]
+    ensureCgen Foo5
+
+  block:
+    type Foo5{.importc.} = array[3, bool]
+    ensureCgen Foo5
+
+  block:
+    type Foo6{.importc, completeStruct.} = object
+      a1: cint
+      # a2: bool # missing this should trigger assert fail
+      a3: cfloat
+      a4: ptr Foo6
+    ensureCgen Foo6
+
+  when false:
+    block:
+      # pre-existing BUG: this should give a CT error in semcheck because `size`
+      # disagrees with `array[3, cint]`
+      type Foo5{.importc, size: 1.} = array[3, cint]
+      ensureCgen Foo5

tests/misc/tsizeof2.nim

@@ -1,5 +1,5 @@
 discard """
-errormsg: "cannot evaluate 'sizeof' because its type is not defined completely"
+errormsg: "'sizeof' requires '.importc' types to be '.completeStruct'"
 line: 9
 """
 

tests/trunner.nim

@@ -19,14 +19,10 @@ proc runCmd(file, options = ""): auto =
     echo result[0]
     echo result[1]
 
-proc testCodegenStaticAssert() =
-  let (output, exitCode) = runCmd("ccgbugs/mstatic_assert.nim")
-  doAssert "sizeof(bool) == 2" in output
-  doAssert exitCode != 0
-
-proc testCTFFI() =
-  let (output, exitCode) = runCmd("vm/mevalffi.nim", "--experimental:compiletimeFFI")
-  let expected = """
+when defined(nimHasLibFFIEnabled):
+  block: # mevalffi
+    let (output, exitCode) = runCmd("vm/mevalffi.nim", "--experimental:compiletimeFFI")
+    let expected = """
 hello world stderr
 hi stderr
 foo
@@ -37,10 +33,30 @@ foo:102:103:104
 foo:0.03:asdf:103:105
 ret={s1:foobar s2:foobar age:25 pi:3.14}
 """
-  doAssert output == expected, output
-  doAssert exitCode == 0
+    doAssert output == expected, output
+    doAssert exitCode == 0
 
-when defined(nimHasLibFFIEnabled):
-  testCTFFI()
 else: # don't run twice the same test
-  testCodegenStaticAssert()
+  template check(msg) = doAssert msg in output, output
+
+  block: # mstatic_assert
+    let (output, exitCode) = runCmd("ccgbugs/mstatic_assert.nim", "-d:caseBad")
+    check "sizeof(bool) == 2"
+    doAssert exitCode != 0
+
+  block: # ABI checks
+    let file = "misc/msizeof5.nim"
+    block:
+      let (output, exitCode) = runCmd(file, "-d:checkAbi")
+      doAssert exitCode == 0, output
+    block:
+      let (output, exitCode) = runCmd(file, "-d:checkAbi -d:caseBad")
+      # on platforms that support _StaticAssert natively, errors will show full context, eg:
+      # error: static_assert failed due to requirement 'sizeof(unsigned char) == 8'
+      # "backend & Nim disagree on size for: BadImportcType{int64} [declared in mabi_check.nim(1, 6)]"
+      check "sizeof(unsigned char) == 8"
+      check "sizeof(struct Foo2) == 1"
+      check "sizeof(Foo5) == 16"
+      check "sizeof(Foo5) == 3"
+      check "sizeof(struct Foo6) == "
+      doAssert exitCode != 0