I've been having issues with compiling Arturo with the latest Nim version and finally I managed to at least spot one piece of code that behaves weirdly.
The piece of code in question (along with different debugging echo statements) is this: https://github.com/arturo-lang/arturo/blob/upgrade-to-nim-2.0.0/src/vm/stack.nim#L65-L73
which gets called from here:
https://github.com/arturo-lang/arturo/blob/upgrade-to-nim-2.0.0/src/vm/lib.nim#L266
(The require template is actually embedded in a proc that gets called at runtime: https://github.com/arturo-lang/arturo/blob/upgrade-to-nim-2.0.0/src/vm/lib.nim#L108)
Now, the whole thing was working flawlessly before; now it doesn't.
And, while try to look into the produced code, I'm seeing this:
// debugEcho "requires >= 1 param"
echoBinSafe(TM__IPU5x6F5i1TFKZ7WkUj8KQ_14, 1);
// let x {.inject.} = move stack.pop()
// let x {.inject.} = move stack.pop()
// let x {.inject.} = move stack.pop()
// debugEcho "popping..."
echoBinSafe(TM__IPU5x6F5i1TFKZ7WkUj8KQ_16, 1);
// SP -= 1
SP__vmZstack_u8 -= ((NI)1);
x = Stack__vmZstack_u7.p->data[SP__vmZstack_u8];
// debugEcho "popping..."
echoBinSafe(TM__IPU5x6F5i1TFKZ7WkUj8KQ_16, 1);
// SP -= 1
SP__vmZstack_u8 -= ((NI)1);
// obj = default(typeof(obj))
eqwasMoved___vmZvaluesZvalue_u2216(&Stack__vmZstack_u7.p->data[SP__vmZstack_u8]);
(which clearly calls pop, twice!)
What is going on here? Is there anything specific related to the latest release that you can point me to that could be the possible culprit?
Interesting idea.
An example of a value passed as spec would be this: https://github.com/arturo-lang/arturo/blob/upgrade-to-nim-2.0.0/src/library/Collections.nim#L59-L62 (the args value).
Do you think there could be something wrong with that?
OK, we've made some progress...
If I remove this move from here: https://github.com/arturo-lang/arturo/blob/upgrade-to-nim-2.0.0/src/vm/lib.nim#L267
Then the whole thing magically works.
But then, I get a copy (eqcopy___), instead of a move (eqwasMoved___).
Ideas?
So, basically the question can be simplified to this...
How do I make this work?
template pop*(): var Value =
SP -= 1
Stack[SP]
# some other template
let x {.inject.} = move stack.pop()
It currently produces:
// let x {.inject.} = move stack.pop()
// SP -= 1
SP__vmZstack_u8 -= ((NI)1);
x = Stack__vmZstack_u7.p->data[SP__vmZstack_u8];
// SP -= 1 <----- This is duplicate!!
SP__vmZstack_u8 -= ((NI)1);
// obj = default(typeof(obj))
eqwasMoved___vmZvaluesZvalue_u2216(&Stack__vmZstack_u7.p->data[SP__vmZstack_u8]);
Looking into the move implementation (and given that my pop is a template), I think the reason why this happens is quite obvious:
https://github.com/nim-lang/Nim/blob/version-2-0/lib/system.nim#L154-L157
You're running into a C code generator double-evaluation bug recently introduced for projects using --gc:arc|orc|atomicArc.
I haven't inspected all callsites of the pop template, but assuming that the popped stack-entries always become re-usable, you can move the move call into the template, like so:
template pop*(): Value =
...
move Stack[SP]
Apart from working around the compiler bug, this also removes the need to use move at pop callsites.
For anyone interested in fixing the bug, the problematic part are the following lines in ccgexprs.nim, introduced by this PR.
The AST for a new =wasMoved call statement is constructed, using the move call's operand as the argument. Generating the code for said call statement then results in the move call's operand being code-gen and emitted twice.
A quick-fix (which might not work with the C++ backend, I haven't tested it) could be replacing the aforementioned lines with:
var b = default(TLoc)
initLocExpr(p, b, newSymNode(op))
linefmt(p, "$1($2);$n", [rdLoc(b), byRefLoc(a)]
However, this doesn't fix the related issue of move behaving differently with the JavaScript and VM backends (no =wasMoved call is inserted there) than compared to with the C backend.
Ultimately, lowering the mMove magic during the injectdestructors pass would be the more robust fix.
Apart from working around the compiler bug, this also removes the need to use move at pop callsites.
That's precisely what I was thinking about right now, after having isolated the issue that is.
Thanks a lot for your input! Really appreciated! :)
@Zerbina I did as you suggested (move the... move inside the template).
It appears to be working beautifully: https://github.com/arturo-lang/arturo/pull/1255
Very nice one. Thanks again! ;-)