From ab5e26c53cf34a1839dd92d247b5e0a5cde432a0 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 7 Apr 2020 06:17:30 -0700 Subject: [PATCH] fix some codegen bugs: NIM_BOOL, NIM_STATIC_ASSERT, --passc:-std=... (etc) (#13798) * fix cgen bool D20200328T203812 * --passc:std=c++17 (etc) now works instead of silently ignored * document caveats for NIM_NIL --- compiler/extccomp.nim | 8 ++-- lib/nimbase.h | 65 +++++++++++++++++++----------- tests/ccgbugs/mstatic_assert.nim | 6 +++ tests/ccgbugs/tcodegenbug_bool.nim | 11 +++++ tests/trunner.nim | 46 +++++++++++++++++++++ tests/vm/tevalffi.nim | 28 ------------- 6 files changed, 110 insertions(+), 54 deletions(-) create mode 100644 tests/ccgbugs/mstatic_assert.nim create mode 100644 tests/ccgbugs/tcodegenbug_bool.nim create mode 100644 tests/trunner.nim delete mode 100644 tests/vm/tevalffi.nim diff --git a/compiler/extccomp.nim b/compiler/extccomp.nim index 307be249eb..9e89b5aa4d 100644 --- a/compiler/extccomp.nim +++ b/compiler/extccomp.nim @@ -612,6 +612,11 @@ proc getCompileCFileCmd*(conf: ConfigRef; cfile: Cfile, # We produce files like module.nim.cpp, so the absolute Nim filename is not # cfile.name but `cfile.cname.changeFileExt("")`: var options = cFileSpecificOptions(conf, cfile.nimname, cfile.cname.changeFileExt("").string) + if useCpp(conf, cfile.cname): + # needs to be prepended so that --passc:-std=c++17 can override default. + # we could avoid allocation by making cFileSpecificOptions inplace + options = CC[c].cppXsupport & ' ' & options + var exe = getConfigVar(conf, c, ".exe") if exe.len == 0: exe = getCompilerExe(conf, c, cfile.cname) @@ -620,9 +625,6 @@ proc getCompileCFileCmd*(conf: ConfigRef; cfile: Cfile, ospNeedsPIC in platform.OS[conf.target.targetOS].props: options.add(' ' & CC[c].pic) - if useCpp(conf, cfile.cname): - options.add(' ' & CC[c].cppXsupport) - var compilePattern: string # compute include paths: var includeCmd = CC[c].includeCmd & quoteShell(conf.libpath) diff --git a/lib/nimbase.h b/lib/nimbase.h index 5b0dcc2c8d..597a5317fe 100644 --- a/lib/nimbase.h +++ b/lib/nimbase.h @@ -262,6 +262,21 @@ __AVR__ #include #include +// define NIM_STATIC_ASSERT +// example use case: CT sizeof for importc types verification +// where we have {.completeStruct.} (or lack of {.incompleteStruct.}) +#if (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L) +#define NIM_STATIC_ASSERT(x, msg) _Static_assert((x), msg) +#elif defined(__cplusplus) +#define NIM_STATIC_ASSERT(x, msg) static_assert((x), msg) +#else +#define NIM_STATIC_ASSERT(x, msg) typedef int NIM_STATIC_ASSERT_AUX[(x) ? 1 : -1]; +// On failure, your C compiler will say something like: +// "error: 'NIM_STATIC_ASSERT_AUX' declared as an array with a negative size" +// we could use a better fallback to also show line number, using: +// http://www.pixelbeat.org/programming/gcc/static_assert.html +#endif + /* C99 compiler? */ #if (defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901)) # define HAVE_STDINT_H @@ -288,34 +303,40 @@ __AVR__ namespace USE_NIM_NAMESPACE { #endif +// preexisting check, seems paranoid, maybe remove +#if defined(NIM_TRUE) || defined(NIM_FALSE) || defined(NIM_BOOL) +#error "nim reserved preprocessor macros clash" +#endif + /* bool types (C++ has it): */ #ifdef __cplusplus -# ifndef NIM_TRUE -# define NIM_TRUE true -# endif -# ifndef NIM_FALSE -# define NIM_FALSE false -# endif -# define NIM_BOOL bool +#define NIM_BOOL bool +#elif (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901) +// see #13798: to avoid conflicts for code emitting `#include ` +#define NIM_BOOL _Bool +#else +typedef unsigned char NIM_BOOL; // best effort +#endif + +NIM_STATIC_ASSERT(sizeof(NIM_BOOL) == 1, ""); // check whether really needed + +#define NIM_TRUE true +#define NIM_FALSE false + +#ifdef __cplusplus # if __cplusplus >= 201103L # /* nullptr is more type safe (less implicit conversions than 0) */ # define NIM_NIL nullptr # else -# /* consider using NULL if comment below for NIM_NIL doesn't apply to C++ */ +# // both `((void*)0)` and `NULL` would cause codegen to emit +# // error: assigning to 'Foo *' from incompatible type 'void *' +# // but codegen could be fixed if need. See also potential caveat regarding +# // NULL. +# // However, `0` causes other issues, see #13798 # define NIM_NIL 0 # endif #else -# ifdef bool -# define NIM_BOOL bool -# else - typedef unsigned char NIM_BOOL; -# endif -# ifndef NIM_TRUE -# define NIM_TRUE ((NIM_BOOL) 1) -# endif -# ifndef NIM_FALSE -# define NIM_FALSE ((NIM_BOOL) 0) -# endif +# include # define NIM_NIL ((void*)0) /* C's NULL is fucked up in some C compilers, so the generated code does not rely on it anymore */ #endif @@ -516,10 +537,8 @@ static inline void GCGuard (void *ptr) { asm volatile ("" :: "X" (ptr)); } # define GC_GUARD #endif -/* Test to see if Nim and the C compiler agree on the size of a pointer. - On disagreement, your C compiler will say something like: - "error: 'Nim_and_C_compiler_disagree_on_target_architecture' declared as an array with a negative size" */ -typedef int Nim_and_C_compiler_disagree_on_target_architecture[sizeof(NI) == sizeof(void*) && NIM_INTBITS == sizeof(NI)*8 ? 1 : -1]; +// Test to see if Nim and the C compiler agree on the size of a pointer. +NIM_STATIC_ASSERT(sizeof(NI) == sizeof(void*) && NIM_INTBITS == sizeof(NI)*8, ""); #ifdef USE_NIM_NAMESPACE } diff --git a/tests/ccgbugs/mstatic_assert.nim b/tests/ccgbugs/mstatic_assert.nim new file mode 100644 index 0000000000..dbf9c03d16 --- /dev/null +++ b/tests/ccgbugs/mstatic_assert.nim @@ -0,0 +1,6 @@ +{.emit:""" +NIM_STATIC_ASSERT(sizeof(bool) == 1, ""); +#warning "foo2" +NIM_STATIC_ASSERT(sizeof(bool) == 2, ""); +#warning "foo3" +""".} diff --git a/tests/ccgbugs/tcodegenbug_bool.nim b/tests/ccgbugs/tcodegenbug_bool.nim new file mode 100644 index 0000000000..a0dbf4eb2d --- /dev/null +++ b/tests/ccgbugs/tcodegenbug_bool.nim @@ -0,0 +1,11 @@ +discard """ +""" + +# issue #13798 +{.emit:""" +#include +void fun(bool a){} +""".} + +proc fun(a: bool) {.importc.} +fun(true) diff --git a/tests/trunner.nim b/tests/trunner.nim new file mode 100644 index 0000000000..7e1468e840 --- /dev/null +++ b/tests/trunner.nim @@ -0,0 +1,46 @@ +discard """ + joinable: false +""" + +## tests that don't quite fit the mold and are easier to handle via `execCmdEx` +## A few others could be added to here to simplify code. + +import std/[strformat,os,osproc,strutils] + +proc runCmd(file, options = ""): auto = + let mode = if existsEnv("NIM_COMPILE_TO_CPP"): "cpp" else: "c" + const nim = getCurrentCompilerExe() + const testsDir = currentSourcePath().parentDir + let fileabs = testsDir / file.unixToNativePath + doAssert fileabs.existsFile, fileabs + let cmd = fmt"{nim} {mode} {options} --hints:off {fileabs}" + result = execCmdEx(cmd) + when false: # uncomment if you need to debug + 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 = """ +hello world stderr +hi stderr +foo +foo:100 +foo:101 +foo:102:103 +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 + +when defined(nimHasLibFFIEnabled): + testCTFFI() +else: # don't run twice the same test + testCodegenStaticAssert() diff --git a/tests/vm/tevalffi.nim b/tests/vm/tevalffi.nim deleted file mode 100644 index 02374869ec..0000000000 --- a/tests/vm/tevalffi.nim +++ /dev/null @@ -1,28 +0,0 @@ -discard """ - joinable: false -""" - -import std/[strformat,os,osproc] - -proc main() = - const nim = getCurrentCompilerExe() - const file = currentSourcePath().parentDir / "mevalffi.nim" - # strangely, --hint:cc:off was needed - let cmd = fmt"{nim} c -f --experimental:compiletimeFFI --hints:off --hint:cc:off {file}" - let (output, exitCode) = execCmdEx(cmd) - let expected = """ -hello world stderr -hi stderr -foo -foo:100 -foo:101 -foo:102:103 -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 - -when defined(nimHasLibFFIEnabled): - main()