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

fix some codegen bugs: NIM_BOOL, NIM_STATIC_ASSERT, --passc:-std=... (etc) #13798

Merged
merged 10 commits into from
Apr 7, 2020

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Mar 29, 2020

  • --passc:-std=c++17 (or similar flags) are now honored instead of silently ignored, by putting --passc options after instead of before the default

  • use correct definition for NIM_BOOL in C target. Previous definition was incorrect, as bool is not guaranteed to be compatible with unsigned char (could have different sizeof etc); furthermore, it would cause codegen error for C code that uses _Bool or (after include <stdbool.h>) bool
    note: it's >= C99 but I doubt nim codegen works with < C99; if needed an extra check (or conditional define) can be used

  • NIM_NIL is incorrect for c++ < c++11, but fixing it would be hard and maybe not worth it, eg:

when true:
  {.emit:"""
  #include <stdio.h>
  struct Foo {
    int fun(void* a) { return 1;}
    int fun(int a){ return 2; }
  };
  """.}
  type Foo {.importcpp.} = object
  proc fun(foo: var Foo, a: pointer): cint {.importcpp.}
  var foo: Foo
  echo foo.fun(nil)
nim cpp -r --passc=-std=c++03 $timn_D/tests/nim/all/t10447.nim
2
nim cpp -r --passc=-std=c++17 $timn_D/tests/nim/all/t10447.nim
1

(requires rebuilding nim with this PR so that --passc=-std=c++03 is honored)

EDIT

links

@timotheecour timotheecour changed the title fix some codegen bugs: bool, --passc:-std=c++17 (etc) fix some codegen bugs: NIM_BOOL, --passc:-std=c++17 (etc) Mar 29, 2020
compiler/extccomp.nim Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Mar 29, 2020

stdbool.h is useless though as it doesn't say anything about sizeof(bool) and Nim requires it to be 1 byte.

@timotheecour
Copy link
Member Author

timotheecour commented Mar 30, 2020

PTAL

stdbool.h is useless though as it doesn't say anything about sizeof(bool) and Nim requires it to be 1 byte.

  • removed stdbool.h and used _Bool directly instead to defined NIM_BOOL, which still fixes issue in top post

  • I'm defining a portable NIM_STATIC_ASSERT and checking NIM_STATIC_ASSERT(sizeof(bool)==1); if a weird platform shows up where sizeof(_Bool)!=1, we'll have a CT error instead of potentially weird bugs and we'll adjust nimbase.h accordingly (eg via #ifdef NIM_DEFINE_BOOL_AS_UCHAR)

  • NIM_STATIC_ASSERT is better (and easier to reuse) than the old hack as it shows line number and failing condition; I added a test for checking its behavior

  • NIM_STATIC_ASSERT is a key ingredient to support CT sizeof for importc types, see importc types: CT sizeof, $, {.sizeof.}, const CVAR {.importc.}: int RFCs#205 for details

  • couldn't figure out how to use the testament discard spec for a test like tests/ccgbugs/mstatic_assert.nim so I'm instead compiling it in tests/trunner.nim (after refactoring that file from tests/vm/tevalffi.nim); the few other tests that also need an explicit compilation step (or non-trivial checks on the output) can also be simplified by merging their logic into trunner (better than making testament spec more complex with more magic flags IMO)

  • related to that, I've fixed a bug so that CTFFI now tests the c++ (instead of C) target depending on NIM_COMPILE_TO_CPP

@timotheecour timotheecour changed the title fix some codegen bugs: NIM_BOOL, --passc:-std=c++17 (etc) fix some codegen bugs: NIM_BOOL, NIM_STATIC_ASSERT, --passc:-std=... (etc) Mar 30, 2020
@Araq
Copy link
Member

Araq commented Apr 1, 2020

it's >= C99 but I doubt nim codegen works with < C99; if needed an extra check (or conditional define) can be used

We support C89.

@timotheecour
Copy link
Member Author

We support C89.

done, that was a simple fix, PTAL

lib/nimbase.h Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member Author

friendly ping @Araq

jlokier added a commit to status-im/nim-evmc that referenced this pull request May 10, 2021
EVMC adopts the C99 standard, and one of its interfaces uses C99 `bool`.

Nim's `bool` uses C `bool` when compiler is in C99 mode (but some default
to C89) and Nim version is >= 1.4.6, or always on C++, but those conditions
aren't always met.  See nim-lang/Nim#13798.

Although `sizeof(bool)` is nearly always 1, not always.  Some targets use
`enum` or `int`, e.g. Apple/Darwin PPC, some 32-bit ARMs.  Anyway, it isn't
guaranteed to be returned from a function the same way as a integer.

So do the right thing and use the C99 `<stdbool.h>` type.  Most targets
have this header even in C89 mode, and if they don't we can't guarantee
EVMC binary compatibility.  The `= bool` tells Nim it's semantically
compatible with the Nim `bool` type, and the `.importc` part says to use
C `bool` in the generated C code.

Signed-off-by: Jamie Lokier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to override -std:gnu++14
3 participants