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

Codegen: Faulty code with global pragma and indirect overload based initialization #5132

Closed
ReneSac opened this issue Dec 19, 2016 · 8 comments · Fixed by #20812
Closed

Codegen: Faulty code with global pragma and indirect overload based initialization #5132

ReneSac opened this issue Dec 19, 2016 · 8 comments · Fixed by #20812

Comments

@ReneSac
Copy link
Contributor

ReneSac commented Dec 19, 2016

Sorry for the horrible title, but I don't fully understand what went wrong.

proc initX(it: float): int = 8

proc main() =
  var f: float
  var x {.global.} = initX(f)
  
main()

gives:

Error: execution of an external compiler program 'gcc -c  -w  -I [redacted] -o /tmp/aporia/nimcache/a7.o /tmp/aporia/nimcache/a7.c' failed with exit code: 1
/tmp/aporia/nimcache/a7.c: In function ‘NimMainModule’:
/tmp/aporia/nimcache/a7.c:110:44: error: ‘f0’ undeclared (first use in this function)
  x_98010_899815578 = initx_98003_899815578(f0);
                                            ^
/tmp/aporia/nimcache/a7.c:110:44: note: each undeclared identifier is reported only once for each function it appears in
> Process terminated with exit code 256

I'm using overload to control my program flow and this function generates the above error. It also give the same error if I replace the float by int too. The following variations of the main functions are fine:

W/o parameters

proc initX(): int = 8

proc main() =
  var f: float
  var x {.global.} = initX()
  
main()

W/o {.global.}

proc initX(it: float): int = 8

proc main() =
  var f: float
  var x = initX(f)
  
main()
@Araq
Copy link
Member

Araq commented Dec 21, 2016

Well that cannot work, so it just needs a better error message.

@ReneSac
Copy link
Contributor Author

ReneSac commented Dec 21, 2016

Why it cannot work but the two bellow can? I don't see what is wrong with that combination.

@Araq
Copy link
Member

Araq commented Dec 21, 2016

proc initX(it: float): int = 8

proc main() =
  var f: float
  var x {.global.} = initX(f)
  
main()

initX(f) depends on a local variable?

@ReneSac
Copy link
Contributor Author

ReneSac commented Dec 21, 2016

Nope, it is dependent just on the type of the variable given to initX. In the manual it says that "When used within a generic proc, a separate unique global variable will be created for each instantiation of the proc."

Here something more similar to what I actually used in my program (gives the same codegen error):

proc initX(unused: float): int = 8

proc foo(bar: float) =
  var x {.global.} = initX(bar)
  echo x, bar
  

let f: float = 2.0
foo(f)

And here a functionally equivalent version that works (and actually may be the perfect workaround for now):

proc initX[T](): int = 8

proc foo[T](bar: T) =
  var x {.global.} = initX[T]()
  echo x, bar
  

let f: float = 2.0
foo(f)

I don't remember anything in the manual that forbids the first form.

EDIT: Actually, not perfect at all. It fails in my actual use case:

proc initX[float](): int = 8

proc initX[int](): int = 1

proc foo[T](bar: T) =
  var x {.global.} = initX[T]()
  echo x, bar
  

let f: float = 2.0
foo(f)

With

/tmp/aporia/a7.nim(11, 4) template/generic instantiation from here

/tmp/aporia/a7.nim(6, 30) Error: ambiguous call; both a7.initX() and a7.initX() match for: ()

While w/o the {.global.} that overloading works:

proc initX(unused:float): int = 8

proc initX(unused: int): int = 1

proc foo(bar: float|int) =
  var x = initX(bar)
  echo x, bar
  

let f: int = 2
foo(f)

@Araq
Copy link
Member

Araq commented Dec 21, 2016

In the manual it says that "When used within a generic proc, a separate unique global variable will be created for each instantiation of the proc."

But there is no generic involved.

And here a functionally equivalent version that works (and actually may be the perfect workaround for now):

It might be equivalent for your use case, but it's completely different from my perspective: It doesn't depend on a runtime value (ignored in the proc body or not doesn't matter).

While w/o the {.global.} that overloading works.

Come on, that's not why it works and you know it. [int] is not the same as [T: int], you gave the generic type parameter the name int, no specialization going on here.

@ReneSac
Copy link
Contributor Author

ReneSac commented Dec 21, 2016

Come on, that's not why it works and you know it. [int] is not the same as [T: int], you gave the generic type parameter the name int, no specialization going on here.

I'm actually coming back to nim after a while away so I'm not totally up to speed. The following code gives exactly the same error:

proc initX[T:float|float32](): int = 8

proc initX[T:int|uint](): string = "bla"

proc foo[T](bar: T) =
  var x {.global.} = initX[T]()
  echo x, bar
  
let f: float = 2.0
foo(f)

I suppose that no specialization is going on there either.

It might be equivalent for your use case, but it's completely different from my perspective: It doesn't depend on a runtime value (ignored in the proc body or not doesn't matter).

Ok, so the compiler isn't supposed to be clever enough to detect that it doesn't actually depends on a runtime value because it is ignored by the called proc. A better error message would indeed help (and ideally some documentation on that limitation would be good too).

So what actually solved my problem is to create a wrapper proc that calls two different plain procs depending on a typedesc parameter. I'm basically doing the overload manually and for this case at least I can manage. This works perfectly:

proc initX(T: typedesc): auto =
  when T is float|float32:
    return initXf()
  else:
    return initXi()

proc initXf(): int = 8

proc initXi(): string = "bla"

proc foo(bar: int|uint|float|float32) =
  var x {.global.} = initX(bar.type)
  echo x, bar
  

let f: float = 2.0
foo(f)

@yglukhov
Copy link
Member

yglukhov commented Dec 21, 2016

AFAIR, C++ does handle this case by emitting initialization code right in the place where its written and protecting it with "once". So runtime value dependancy is not a problem at all. Also such behavior looks more intuitive to me. I was really surprised that initialization code is executed long before the var definition is encountered. So to summarize my opinion: changing codegen to produce lazy initializer looks both more logical to me plus it fixes this issue.

@zah zah self-assigned this Mar 17, 2017
@stale
Copy link

stale bot commented Aug 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. If you think it is still a valid issue, write a comment below; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Aug 4, 2020
@stale stale bot removed the stale Staled PR/issues; remove the label after fixing them label Apr 1, 2021
bung87 added a commit to bung87/Nim that referenced this issue Nov 11, 2022
Araq added a commit that referenced this issue Nov 12, 2022
…20812)

* fix #3505 wrong var {.global.} initialization, asign variable to it

* fix #5132 as well

* follow suggestions

* handle all call kinds

* Update tests/global/t3505.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* follow suggestion

* Update compiler/semstmts.nim

Co-authored-by: Andreas Rumpf <[email protected]>
capocasa pushed a commit to capocasa/Nim that referenced this issue Mar 31, 2023
… to it (nim-lang#20812)

* fix nim-lang#3505 wrong var {.global.} initialization, asign variable to it

* fix nim-lang#5132 as well

* follow suggestions

* handle all call kinds

* Update tests/global/t3505.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* follow suggestion

* Update compiler/semstmts.nim

Co-authored-by: Andreas Rumpf <[email protected]>
narimiran pushed a commit that referenced this issue Apr 25, 2023
…20812)

* fix #3505 wrong var {.global.} initialization, asign variable to it

* fix #5132 as well

* follow suggestions

* handle all call kinds

* Update tests/global/t3505.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* follow suggestion

* Update compiler/semstmts.nim

Co-authored-by: Andreas Rumpf <[email protected]>
(cherry picked from commit 1410243)
narimiran pushed a commit that referenced this issue Apr 25, 2023
…20812)

* fix #3505 wrong var {.global.} initialization, asign variable to it

* fix #5132 as well

* follow suggestions

* handle all call kinds

* Update tests/global/t3505.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* follow suggestion

* Update compiler/semstmts.nim

Co-authored-by: Andreas Rumpf <[email protected]>
(cherry picked from commit 1410243)
narimiran pushed a commit that referenced this issue Apr 25, 2023
…20812)

* fix #3505 wrong var {.global.} initialization, asign variable to it

* fix #5132 as well

* follow suggestions

* handle all call kinds

* Update tests/global/t3505.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* follow suggestion

* Update compiler/semstmts.nim

Co-authored-by: Andreas Rumpf <[email protected]>
(cherry picked from commit 1410243)
narimiran pushed a commit that referenced this issue Apr 25, 2023
…20812)

* fix #3505 wrong var {.global.} initialization, asign variable to it

* fix #5132 as well

* follow suggestions

* handle all call kinds

* Update tests/global/t3505.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* follow suggestion

* Update compiler/semstmts.nim

Co-authored-by: Andreas Rumpf <[email protected]>
(cherry picked from commit 1410243)
narimiran pushed a commit that referenced this issue Apr 26, 2023
…20812)

* fix #3505 wrong var {.global.} initialization, asign variable to it

* fix #5132 as well

* follow suggestions

* handle all call kinds

* Update tests/global/t3505.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* follow suggestion

* Update compiler/semstmts.nim

Co-authored-by: Andreas Rumpf <[email protected]>
(cherry picked from commit 1410243)
bung87 added a commit to bung87/Nim that referenced this issue Jul 29, 2023
… to it (nim-lang#20812)

* fix nim-lang#3505 wrong var {.global.} initialization, asign variable to it

* fix nim-lang#5132 as well

* follow suggestions

* handle all call kinds

* Update tests/global/t3505.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* Update compiler/semstmts.nim

* follow suggestion

* Update compiler/semstmts.nim

Co-authored-by: Andreas Rumpf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants