fixes #1987; show template calls as inlined frames when debugging

[Redict]

Stepping over a template call jumps into the template's body in another file, with no frame to attribute it to. This carries the expansion's provenance to the debug backend, which turns it into DWARF DISubprogram + inlinedAt.

Rewritten to the filename encoding you asked for, so the comesfrom tag is gone: no node, no dispatcher branches, and #2255 is no longer needed. Expanded code gets a forged name:

__crucial\0setElem.0.foo\1foo.nim\115\0[]=.0.system\1system.nim\133\0system.nim
^prefix   ^--------- outermost ---------^ ^--------- innermost --------^ ^real

The chain is outermost-first, so its length is the inlining depth. Everything after the last NUL is the real file, which is what realFile() returns for every consumer that does not care about frames.

Two things beyond the sketch

Each entry carries <sym>\1<declfile>\1<declline>, not just the symbol. The declaration site cannot be recovered downstream: a template decl does not survive into Leng, and the expanded code's own info points at wherever the body came from. Without it a frame for a template declared in the test file claimed to live in system.nim. This also makes the result better than the wrapper design, which could only reach the body's first statement and was one line off.

\1 rather than | as the field separator, since Nim lets an operator be named |.

Result

template setElem(x: ptr UncheckedArray[char]; i: int; elem: char) =
  x[i] = elem

proc run(p: ptr UncheckedArray[char]) =
  setElem(p, 0, 'H')

!11 = distinct !DISubprogram(name: "setElem", file: !2, line: 15)
!12 = distinct !DISubprogram(name: "[]=", file: !7, line: 33)
!13 = !DILocation(line: 34, scope: !11, inlinedAt: !9)
!14 = !DILocation(line: 34, scope: !12, inlinedAt: !13)

setElem expands []=, so the chain nests two deep, and each frame points at its own declaration.

Cost

system.nim's .s.nif grows 4.01%, from 63 distinct forged names covering every expansion in the module. Filenames are interned, so a chain is paid for once per distinct expansion rather than per token. I had assumed this would be much worse before measuring it.

Four things had to learn about the encoding

• visibilityModule compares the real file. It judges private-field access by which module code was written in, and a forged name has its own FileId, so raw id comparison missed and tfieldvisplugin broke.
• reporters.infoToStr and the other path printers call realFile, or an error message leaks the encoding to the user.
• (err ...) subtrees are copied verbatim. Their dot tokens are the instantiation contexts behind Trace: instantiation from here; re-emitting them duplicated the trace for a template erroring inside another expansion (tinvalidrecursion).
• Dot tokens elsewhere keep their line info. buildErr stores the error contexts there, so dropping it silently lost every trace line.

The forge runs after the body is sem-checked, not before: a template called inside this one has already expanded and forged its own name by then, so the outer level prepends onto the existing chain and the order comes out outermost-first, which is what nesting inlinedAt needs.

One more constraint worth recording: nimony has no closures, so the pass cannot use captured locals. hastur boot is what catches that.

Tests

tests/llvmdebug, a golden suite over the .ll's debug metadata and the first tests for the debug backend. Two cases: the nested expansion above, and a var declared inside a template, where the DILocalVariable scope and its DILocation must agree or the verifier rejects the module. Both verified with llvm-as.

hastur.mode = skip since the LLVM backend cannot build the full stdlib yet; run with hastur tests/llvmdebug.

672/672 and hastur boot. tvarargs.nif, tresemtype.nif and ttemplate.nif are re-recorded: toplevel echo is a varargs template, so its expansion now carries a forged name.

Not done

intramodinliner wraps inlined bodies in (scope ...) and has the same missing-inlinedAt bug. It can reuse this encoding as-is, since nothing about it is template-specific.

[Araq]

Instead of tmplbody use the tag name comesfrom.

[Redict]

Renamed, pushed as a fourth commit.

Better name than mine: it says the statements came from expanding something, without claiming what. intramodinliner has the same missing-inlinedAt bug and can reuse this tag as-is now, instead of choosing between a template-named tag and a second one.

Comments that read "the template's symbol" now say "the origin symbol" where the code isn't template-specific. sem.nim's re-sem branch keeps the template wording, since that path genuinely is semTemplateCall's.

Tag value stays 348. The .ll goldens are unchanged - the tag name never reaches DWARF. 644/644, llvmdebug 2/2.

[Redict]

Rebased on master, resolving against my own visOwner change from #2230. It wraps semExpr; this one wraps the emitted statements, so they nest.

The rebase also surfaced a third pass with the same bug as derefs/vl: shoggoth/cse walked the wrapper as an expression and treated it as a hoist anchor. useCpuRegisters in nifreader.nim declares an {.inject.} p and loads ^p, both inside the wrapper, so CSE hoisted the load above the declaration and stage 1 failed to compile itself with 'p_11' undeclared. Fixed in the backend commit.

Worth noting hastur boot was the only thing that caught it - the 655-test suite was green.

CI then found a fourth: arkham, in the sibling nativenif repo, asserts on the tag in four passes of its own. Fixed in nim-lang/nativenif#95, which should land with or before this.

That is five passes across two repos, each found by breakage in a different suite, and all for the same reason: (comesfrom SYM S*) puts an operand where every generic walker expects a statement. (pragmax pragmas body) has the identical shape and paid the same tax years earlier - 9 hand-written special cases today. I have written up a proposal for a shared OperandHeadedS shape set plus a validator check so the next such tag costs nothing, and will file it separately rather than grow this PR further.

655/655 and boot pass locally.

[Araq]

Sorry for this late change-of-mind but comesfrom should be a pure line-info carrying node that is the first child of the existing ExprX construct, this should simplify things quite a bit.

[Redict]

A pure line-info leaf is clearly better than a wrapper, and it kills the whole operand-headed problem: nothing has to step over a leading symbol, so the 16 dispatcher branches and the four regressions all go away. #2255 becomes unnecessary.

Before I rewrite it, one thing does not fit and I would rather ask than guess.

expr is NimonyExpr only, not NimonyStmt (doc/tags.md:278, (expr XS+ X)), and there is no ExprS. But the expansions being marked are exactly the void ones, which sit in statement position. semcall.nim only wraps when returnType.typeKind == VoidT, precisely because a non-void expansion is an expression and beforeCall has to keep pointing at the value for commonType. In the current output that shows up as

(comesfrom@,e echo.0.
 (stmts@2,~6
  (stmts@2,1 (cmd write.1. (haddr@6 stdout.0.)"a"@1,5))
  ...

sitting directly among statements, holding stmts. So (expr (comesfrom …) …) cannot go where the marker is needed today without expr also becoming a statement kind.

Three readings, and I do not know which you mean:

1. Make expr a statement kind too, then a void expansion emits (expr (comesfrom SYM) <stmts…>). Fits the "first child of the existing ExprX" wording literally. Touches every of ExprX site, though most already loop while n.hasMore over the children and treat only the last as the value.

2. Keep it statement-position but as a leaf, so (comesfrom SYM) is emitted as a sibling before the expanded statements rather than wrapping them. Line-info only, no nesting, nothing to descend into. This is the smallest change and gets the whole benefit, but it is not "first child of ExprX".

3. Mark the call site instead of the expansion, wrapping non-void expansions where an (expr …) already exists and dropping the void case for now.

I lean 2 if the goal is a pure line-info node, and 1 if (expr …) in statement position is something you want anyway. Which did you have in mind?

Worth flagging on cost either way: the tag's value is derived from its row index in doc/tags.md, so changing its shape re-records the .nif goldens regardless. That part is mechanical. What I want to avoid is rewriting it twice.

Separately, nim-lang/nativenif#95 is now guarded with when declared(ComesfromS) so arkham compiles against a nimony with or without the tag. That breaks the CI deadlock between the two repos independently of which shape wins here.

[Araq]

There is a better design still. Capturing line information well was a first class criterion when designing NIF. All we need to do here is to forge a special filename that encodes the real filename as well as const CrucialPrefix = __crucial\0 prefix (with a binary zero that filenames cannot contain otherwise)

Then LLVM etc can simply extract the comesfrom information by n.info.filename.startsWith(CrucialPrefix). The rest of the compilation pipeline must keep line information as precise as possible but this is an existing requirement!

[Redict]

This one removes the tag entirely, which takes #2255 with it. I checked the mechanism holds:

needsEscape is c < ' ' or c in ControlChars (nifbuilder.nim:103), so a \0 in a filename serializes as \00 and round-trips through text NIF. bif.nim writes filenames length-prefixed, so binary is fine too. The LLVM side already keys off info.file: dbgLocationId calls getOrCreateDIFile(c, rawInfo.file), and getOrCreateInlineSP derives the frame's file and line from the expansion body's own info. Both would read the prefix instead of a pushed frame.

Two things a prefix alone does not carry.

The synthetic DISubprogram is named after the template and cached per SymId, so every call site of echo shares one frame instead of making a new one each time. A filename says the statements came from somewhere, not from what. Encoding the symbol into the forged name (__crucial\0echo.0.\0<realfile>) carries it, at one interned filename per expanded symbol per module. Otherwise the SP gets named after the file and loses per-template identity.

Nesting is the other. setElem expands []=, so the goldens have !13 -> inlinedAt !10 -> inlinedAt !7, two levels, currently a stack of frames. With a flat filename the depth has to live in the encoding too or nested expansions collapse into one. __crucial\0a\0__crucial\0b\0real.nim would do it and is not pretty.

Neither is fatal, but both push toward encoding a small record in the filename, so I would rather have your call than my guess.

On keeping line information precise: it is an existing requirement, but it is not currently met on this path, and that is the real risk here. hexer and the Leng passes rewrite statements freely, and the info that reaches lengc for an expanded body is frequently the call site's, not the template body's. That is why the current patch carries provenance in a node instead of inferring it, and why getOrCreateInlineSP documents the body's own info as the only surviving source for the template's file and line. As a filename, every pass that reassigns info has to preserve the forged one, and a pass that substitutes the call site's info produces a frame pointing at the wrong file with nothing to catch it. The node form fails loudly instead.

Happy to build it either way. Since it is the third shape, I would rather pin the encoding down before re-recording the goldens again.

[Araq]

Instead of __crucial\0a\0__crucial\0b\0real.nim use __crucial\0a\0b\0real.nim and then go for this solution because:

Statement-only inling expansion information is not good enough, we want to do this for every template expansion and expression producing templates are very common. This means that any explicit node-based solution would pay a heavy implementation price as expressions are more frequent than statements.

[Araq]

Move all of this logic to a new module named src/lib/comesfrom.nim

[Redict]

Done: src/lib/comesfrom.nim. It has no NIF dependency, just string handling, so nifpools and nifcoreparse re-export it and every consumer keeps reaching realFile without a separate import.

[Araq]

What?! Why? Sounds terrible. Only the templates body's first token needs the comesfrom information.

[Redict]

I tried it. Forging only the body's first token gives zero inlined frames:

!5  = DISubprogram(name: "run2", ...)
!21 = DISubprogram(name: "X60Qini_0_ttewk27gl1", ...)
!49 = DISubprogram(name: "main", ...)

No withTemp subprogram, no inlinedAt anywhere, and tests/llvmdebug drops to 0/1. The forged name does reach the .c.nif, exactly one occurrence on the head, so nothing is losing it in the pipeline.

What breaks is the consumption side. The backend has no notion of a current node to inherit from: setLoc is called from 46 places, each reading info off whatever cursor that lowering happens to hold, and emit stamps currentProc.dbgLoc onto every instruction. An instruction lowered from the third statement of a template body therefore gets that statement's own info. With only the head marked, everything after the first instruction falls back to the enclosing proc and the frame collapses.

So head-only needs the backend to track the active expansion across a subtree walk, which is a frame stack in lengc - the wrapper node again, without the node. Your call which way to go; I did not want to guess a fourth time.

On the cost, since that is the part that sounded terrible: filenames are interned. All of system.nim comes to 63 distinct forged names covering 8897 token references, and the .s.nif grows 4.01%. The re-emit is a single pass over the expansion buffer at expansion time. Per distinct expansion, not per token.

Both review comments are in: the logic moved to src/lib/comesfrom.nim, and I took the else: branch. The committed version of it has else: one space too deep and does not compile - templates.nim(145, 6) Error: invalid indentation - so my commit fixes the indentation on top of yours.

[Araq]

Make no sense to me, in the codegen trExpr before it recurses it checks for a comes-from line information, pushes it onto a stack or whatever and pops it later, bam, done, the backend has DISubprogram information.

[Redict]

Doing it that way. The stack in the walker means only the head of an expansion needs a forged name, so the per-token re-emit goes away and most of the size growth should go with it. I'll push once tests/llvmdebug passes again.