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

bugs with {.emit.} #13943

Closed
6 tasks
timotheecour opened this issue Apr 10, 2020 · 5 comments
Closed
6 tasks

bugs with {.emit.} #13943

timotheecour opened this issue Apr 10, 2020 · 5 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Apr 10, 2020

summary

  • A1 emit: "foo `someNimVar` bar".} can sometimes give wrong runtime results (which is worse than CT errors); the newer syntax emit: [..] doesn't have that issue though; EDIT: this happens in particular in templates and is related to "undeclared identifier" error when using fmt from strformat on devel inside a template  #10977 and asm code in templates fail #2362

  • A2 emit doesn't work with var variables, but works with result pseudo-var-variable

  • A3 block scope emit should not be relocated

  • A4 {.emit:"/*HERE*/ do not move me".} should be introduced (trivial change); would be useful for module-scope emit's even if block scope emit bug is fixed

  • A5 tsizeof.c_sizeof shows a seemingly correct usage of emit that is in fact incorrect and would be easily fixed with /*HERE*/

  • A6 nim cpp differs from nim c (related to A2)

Example 1

import unittest

template testEmit0(cond: string, body) =
  {.emit: ["""#if """, cond, "\n"].} # OK: this works
  body
  {.emit: "#endif\n".}

template testEmit1(cond: string, body) =
  # {.emit: ["""#if """, cond, "\n"].} # OK: this works
  {.emit: "#if `cond`\n".} # BUG: `cond` generated as `cond` litteral!!
  body
  {.emit: "#endif\n".}

proc testEmit2(count: var int) =
  {.emit: """`count` = 3;""".}

proc testEmit3(count: int): int =
  var x = 0
  {.emit: """`x` = `count`;""".}
  return x

proc testEmit4(count: int): int =
  {.emit: """`result` = `count`;""".}

proc main() =
  var count = 0

  # BUG1 D20200409T213938:here quote replacement doesn't work in templates
  testEmit1("true"):
    count = 7
  check count == 7

  # ok: quote replacement works inside procs
  doAssert testEmit3(11) == 11

  # ok: emit with `[]` works in side templates
  testEmit0("true"):
    count = 9
  doAssert count == 9

  # BUG2 D20200409T214500:here (or at least caveat that should be documented): emit replacement doesn't work with `var` variables
  testEmit2(count)
  check count == 3

  # ok: it works with `result` magic variable even though it's kind of like a var variable
  doAssert testEmit4(4) == 4

main()

Current Output

/Users/timothee/git_clone/nim/timn/tests/nim/all/t10528.nim(35, 14): Check failed: count == 7
count was 0
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10528.nim(47, 14): Check failed: count == 3
count was 9

Expected Output

works

Example 2

# D20200409T215527:here
template cstaticIf(cond: string, body) =
  echo "#if " & cond
  {.emit: ["""#if """, cond, "\n"].} # {.emit: "#if `cond`\n".} would hit D20200409T213938
  body
  {.emit: "#endif\n".}
  echo "#endif " & cond & "\n"

template fun() =
  cstaticIf "__cplusplus >= 201703L":
    echo "enabled"

  cstaticIf "__cplusplus < 201703L":
    echo "enabled"

echo "moduleLevel"
fun()

template blockLevel()=
  block:
    echo "blockLevel"
    fun()
blockLevel()

proc procLevel()=
  echo "procLevel"
  fun()
procLevel()

this prints:

moduleLevel
#if __cplusplus >= 201703L
enabled
#endif __cplusplus >= 201703L

#if __cplusplus < 201703L
enabled
#endif __cplusplus < 201703L

blockLevel
#if __cplusplus >= 201703L
enabled
#endif __cplusplus >= 201703L

#if __cplusplus < 201703L
enabled
#endif __cplusplus < 201703L

procLevel
#if __cplusplus >= 201703L
#endif __cplusplus >= 201703L

#if __cplusplus < 201703L
enabled
#endif __cplusplus < 201703L
  • the procLevel output is correct: the code is emitted where the emit is declared

  • BUG: the blockLevel output is incorrect: the code should not be moved around to a separate section because it's inside a block
    it results in buggy behavior as illustrated here

  • the moduleLevel output is technically correct: the code is moved around to a separate section. However we need a new type section modifier:

{.emit: "/*HERE*/ don't move this around".}

in addition to the existing /*TYPESECTION*/, /*VARSECTION*/, /*INCLUDESECTION*/.
It should be a trivial change in determineSection

Example 3

this is related to Example 2.
Here's the implementation from tsizeof.c_sizeof:

# D20200409T220623:here
import macros
macro c_sizeof(a: typed): int32 =
  ## Bullet proof implementation that works using the sizeof operator
  ## in the c backend. Assuming of course this implementation is
  ## correct.
  result = quote do:
    var res: int32
    {.emit: [res, " = sizeof(", `a`, ");"] .}
    res

block:
  let a = c_sizeof(cint)
  echo a

nim c -r -f --stacktrace:off $timn_D/tests/nim/all/t10527b.nim
works, but shouldn't, because warnings are squashed => we really need #11591 which would enable showing those warnings via --warning:BackendWarning:on

nim cpp -r -f --stacktrace:off $timn_D/tests/nim/all/t10527b.nim
this one fails (as it should) and illustrates the bug, which is that emit gets moved around to another section even though it's inside a block:
here's the generated code:

...
resX60gensym16846041___RP9bMW8EVqo5H9c0lD6uqxDQ = sizeof(int);
...
N_LIB_PRIVATE NI32 resX60gensym16846041___RP9bMW8EVqo5H9c0lD6uqxDQ;

and here's the cgen error:

/Users/timothee/git_clone/nim/timn/build/nimcache/@mt10527b.nim.cpp:36:1: error: C++ requires a type specifier for all declarations
resX60gensym16846041___RP9bMW8EVqo5H9c0lD6uqxDQ = sizeof(int);
^
/Users/timothee/git_clone/nim/timn/build/nimcache/@mt10527b.nim.cpp:47:20: error: redefinition of 'resX60gensym16846041___RP9bMW8EVqo5H9c0lD6uqxDQ'
N_LIB_PRIVATE NI32 resX60gensym16846041___RP9bMW8EVqo5H9c0lD6uqxDQ;

example 4: nim cpp differs from nim c

nim c returns a: 262398120 (incorrect)
nim cpp returns a: 10 (correct)

when true: # D20200414T195401:here
  {.emit: "#include <stdio.h>".}
  proc testEmit1b(a: var cint) =
    {.emit: """printf("a: %d\n", `a`);""".}
  var a = 10.cint
  testEmit1b(a)

Additional Information

@krux02
Copy link
Contributor

krux02 commented Apr 10, 2020

tsizeof.c_sizeof shows a seemingly correct usage of emit that is in fact incorrect and would be easily fixed with /HERE/

Can you elaborate a bit more on what the problem is? This test is one of the most important test that I wrote, so it would be good if it is correct.

Unrelated to that, I would really like to have an emit prgama that has an enum argument to control the location. You know the usual problems with these magic comments. No error or warning when you mistype them. No error if the version of the compiler you are using doesn't support that value. No self explanatory API.

@timotheecour
Copy link
Member Author

Can you elaborate a bit more on what the problem is?

that's shown in Example 3; c_sizeof as you wrote it is only valid if used inside a proc (ie not at module or block scope, even if through templates of course); that's because cgen currently inserts the emitted code at file level instead of proc level; it "accidentally" works for C (with bad warnings) because C allows writing res = sizeof(int); without res being declared, but it breaks for C++.

I'm fixing this and other issues in #13953 (see c_sizeof in backendutils)

Unrelated to that, I would really like to have an emit prgama that has an enum argument to control the location. You know the usual problems with these magic comments. No error or warning when you mistype them. No error if the version of the compiler you are using doesn't support that value. No self explanatory API.

also fixing that in #13953

@krux02
Copy link
Contributor

krux02 commented Apr 11, 2020

but it breaks for C++.

but tsizeof is tested in C++ mode

@timotheecour
Copy link
Member Author

but tsizeof is tested in C++ mode

it works in your test because you're only calling c_sizeof inside a proc. If I simply change:
proc foobar() = to template foobar() =, nim cpp -r tests/misc/tsizeof.nim will fail as described. Yes, this is a private test (and c_sizeof is private) but it just illustrates the problem with emit at module/block scope, which I'm fixing in that PR.

@Araq
Copy link
Member

Araq commented Aug 27, 2023

That's just you not understanding emit.

@Araq Araq closed this as completed Aug 27, 2023
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

No branches or pull requests

3 participants