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

[timotheecour]

marking as WIP only because I need to handle failures resulting from ABI checks (which are legit), but ready for review apart from that

• implements proposal 1 from importc types: CT sizeof, $, {.sizeof.}, const CVAR {.importc.}: int RFCs#205 with a minor modification: using {.completeStruct.} instead of 0-argument {.sizeof.} as it's more self documenting and {.sizeof.} was a bit of a misnomer given that offsetof and alignof are involved too.
• for a {.importc, completeStruct.} type, we can now compute sizeof, alignof, offsetof at CT. sizeof is checked at cgen CT (ABI static check)
• for a {.importc.} type, CT sizeof(etc) still gives CT error
• lots of tests, dispatched by tests/trunner.nim:

tests/ccgbugs/mabi_check.nim
tests/ccgbugs/mimportc_size_check.nim

• cgen ABI checks give user friendly errors to make debugging easy, 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)]" (see tests where declaration comes from; it shows both C name (unsigned char), nim type name (BadImportcType) and resolved type name (int64), and declaration location

• {.emit.} now goes in section NIM_merge_EMITS right above NIM_merge_TYPE_INFO (unless user overrides with {.emit:"""/*TYPESECTION*/....} as usual), instead of on top of NIM_merge_PROC_HEADERS which is right after NIM_merge_TYPE_INFO; this makes sense because ABI static checks (which is what NIM_merge_TYPE_INFO contains) should appear after type declarations in emit sections.

• -d:checkAbi is now enabled by default thanks to the portable macro NIM_STATIC_ASSERT introduced in fix some codegen bugs: NIM_BOOL, NIM_STATIC_ASSERT, --passc:-std=... (etc) #13798
  see tests for more examples, but the following would trigger cgen static asserts (with human friendly errors):

type Foo2AliasBad{.importc: "struct Foo2", size: 1.} = object
 a: cint
type Foo5{.importc.} = array[4, cint] # assuming `typedef int Foo5[3];`
type Foo6{.importc, completeStruct.} = object
  a1: cint
  # a2: bool # missing this should trigger assert fail
  a3: cfloat
  a4: ptr Foo6
  discard Foo6.default

semi related

• cgen files now show section top level comments, which makes it easier to figure out where cgen'd code comes from, eg:

/* section: NIM_merge_PROC_HEADERS */
...
/* section: NIM_merge_DATA */
...
/* section: NIM_merge_PROCS */
...

(intentionally left as is instead of DATA etc which is less searchable when figuring out where it comes from in compiler code)

benefits of static checks

this PR actually finds a number of pre-existing errors where compiler is wrong about size of certain types, which can lead to bugs

[Araq]

I love the additional static checks but I wonder why you picked this solution over simply expanding what we already have with .incompleteStruct. The existence of incompleteStruct means that previously other object .importc declarations are considered to be complete and I cannot see what's wrong with this idea. We should catch resulting bugs via the additional static checks, right?

[timotheecour]

I love the additional static checks but I wonder why you picked this solution over simply expanding what we already have with .incompleteStruct. The existence of incompleteStruct means that previously other object .importc declarations are considered to be complete and I cannot see what's wrong with this idea. We should catch resulting bugs via the additional static checks, right?

as explained in detail in timotheecour#97, .incompleteStruct assumes the wrong default (that a importc type is complete unless marked with .incompleteStruct), which is flat-out wrong and dangerous. Instead, this PR makes CT sizeof opt-int rather than opt-out by making the reverse assumption for importc types.

in all of stdlib, there are only 2 types marked as incompleteStruct: CFile and DIR

yet, as you can see with timotheecour#97, almost all types I tried are in fact incomplete:

nim c -r --stacktrace:off -d:nimImplicitCompleteStruct -f $timn_D/src/timn/exp/abichecks.nim

Dirent struct dirent => 272 vs 1048
Tflock struct flock => 32 vs 24
Glob glob_t => 24 vs 88
Group struct group => 24 vs 32
Ipc_perm struct ipc_perm => 20 vs 24
Stat struct stat => 104 vs 144
Statvfs struct statvfs => 88 vs 64

[Araq]

Well... ok I guess but then why do we need .completeStruct? If the struct is complete, don't import it from C...

[timotheecour]

Well... ok I guess but then why do we need .completeStruct? If the struct is complete, don't import it from C...

what you're suggesting cannot work. This would generate distinct types, causing all sorts of problems.
eg, in C you may not notice because warnings are gagged (but with my PR #11591 and --warning:BackendWarning:on you'd see warnings like:

warning: incompatible pointer types passing 'tyObject_Foo__9arDqaKhGY39cZxjiafQj6hg *' (aka 'struct tyObject_Foo__9arDqaKhGY39cZxjiafQj6hg *') to parameter of type 'Foo *' (aka 'struct Foo *') [-Wincompatible-pointer-types]

in C++ this would simply not work at all, even without overloads or templates, you can't implicitly convert;
even in the following case which only contains extern "C" declarations it will not work:

# in t10533.nim
when true:
  const header = "t10533_case5.h"
  {.compile: "t10533_case5.cpp".}
  type Foo = object # BUG
  # type Foo {.importc, header: header.} = object # this would work
    x: cint
    y: cint
  proc fun1(a: ptr Foo) {.importc, header: header.}
  var a = Foo(x: 12, y: 13)
  fun1(a.addr)

# in t10533_case5.cpp
#include "t10533_case5.h"
extern "C"{ void fun1(Foo* a){} }

# in t10533_case5.h
extern "C"{
typedef struct Foo{
  int x;
  int y;
} Foo;
void fun1(Foo* a);
}

would error:

error: no matching function for call to 'fun1'
        fun1((&a__g9abQv8QiP5sIwSJr0L9bK3g));
        ^~~~
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10533_case5.h:6:6: note: candidate function not viable: no known conversion from 'tyObject_Foo__9arDqaKhGY39cZxjiafQj6hg *' to 'Foo *' for 1st argument

and then there are other issues for C as well, eg ndi maps, debugging etc where the same type assumption would break

[krux02]

@Araq:

Well... ok I guess but then why do we need .completeStruct? If the struct is complete, don't import it from C...

Even if it would work. It is dangerous to do so. If you cut the ties to the original C declaration, it isn't possible to add static checks anymore to ensure that both C and Nim share the same stuct definition/layout.

I have to side with @timotheecour here. Imported types are by default incomplete. That is the assumption in sizeof/alignof computation as well. A lot of code has been written this way. I wrote my wrappers this way. If you change this, it is a breaking chage. The incompleteStruct struct pragma is pretty much out of date. And it has been this way since I came to Nim

@timotheecour you should also enable an align annotation, for example this:
type Foo2AliasBad{.importc: "struct Foo2", size: 1, align:1.}. As I mentioned earlier, only if you know size and alignment, offset computations for object members can be computed.

specifing the size for a struct reuses the size pragma that is used for enums as well. Please add some checks that you don't break anything for enums. Please also add a test that if you only specify the size of a struct, that the aligment field isn't also set, (unlike enums, where the alignment is inherited from integer aligment). Also it would be beneficial if you add a simple check that ensures that alignment values are a power of 2 and the size is a multiple of the alignment. These are just sanity checks to catch compilation errors before the C compilation stage.

[Araq]

Imported types are by default incomplete. That is the assumption in sizeof/alignof computation as well. A lot of code has been written this way. I wrote my wrappers this way. If you change this, it is a breaking chage. The incompleteStruct struct pragma is pretty much out of date. And it has been this way since I came to Nim

Yeah, uhm, that's what I tried to say too, I agree with you. I'm confused. Will think about it.

[Araq]

Merging, but this needs a changelog entry and needs to be documented well.

[timotheecour]

Merging, but this needs a changelog entry and needs to be documented well.

on my TODO list

#13926 (comment)

static_assert can also check for alignof (etc) indeed; should be done in future PR's