Expose the interpreter subroutine call stack in task status - #4517
Conversation
Task status carried only callLevel, so a GUI could tell that a subroutine was
running but not which one, nor where it was called from. Worse, callLevel was
sampled from the live interpreter at interp_list dequeue time, so it described
wherever the interpreter had read ahead to rather than the move the machine was
actually executing -- the two are decoupled by up to [TASK]INTERP_MAX_LEN queued
canon commands plus the motion queue.
Rather than reading the interpreter's current state, recover the stack that was
active when the executing move was interpreted:
- The interpreter records every subroutine call as a call_stack_node holding the
call site (filename, line), the subroutine name, and the id of its caller --
a linked chain, so a whole stack is addressable by one integer. Nodes live in
a fixed ring of INTERP_CALL_STACK_NODES (16384, ~512kB of non-realtime memory)
and each stores its own id, so a node whose slot has been reused is detected on
lookup instead of being reported as some unrelated call. enter_context()
pushes, leave_context() pops, unwind_call() resets to the root.
- write_state_tag() stamps the current node id into every block's StateTag as
GM_FIELD_CALL_STACK_ID. It rides through segment merging and TP blending like
the other tag fields.
- InterpBase gains resolve_call_stack_depth(node_id) and
resolve_call_stack_frame(node_id, level, ...), with defaults that report an
empty stack; Interp implements them by walking the node chain. A chain that
cannot be resolved in full reports depth 0 rather than a partial stack against
a truncated depth.
- emcTaskUpdate() resolves the tag of the move motion is executing and fills
EMC_TASK_STAT::callStack[], deriving callLevel from the same id so depth and
frames always describe one point in the program. The stack is cleared once
the interpreter goes idle. emcTaskExecute() no longer writes callLevel.
EMC_TASK_STAT grows EmcCallFrame {filename, subname, line} and callStack[] of up
to EMC_MAX_CALL_STACK frames. Frame[i] reads "at line L of file F we called
subroutine S", i == 0 being the call made from the main program.
That growth pushes EMC_STAT past the 10240-byte emcStatus NML buffer, which
would make NML::write() silently drop status and leave LinuxCNC looking hung to
every GUI, with nothing failing at build time. configs/common/{client,server}.nml
go to 20480, and emcops.cc gains a static_assert on sizeof(EMC_STAT) so the build
trips before that can happen again.
Exposed to Python as stat.call_stack, a tuple of dicts with 'filename',
'subname' and 'line' keys.
tests/interp/call-stack runs a program whose subroutines sit in separate files
and checks that stat.call_stack still names outer/inner while the machine is
cutting inside them -- the point at which the live interpreter has already read
to EOF, and the case the old callLevel got wrong.
Two bounds fixes found along the way, both reachable before this change:
enter_context() incremented call_level before testing it against
INTERP_SUB_ROUTINE_LEVELS, leaving it one past the end of sub_context[] on the
error path, which unwind_call() then indexed; and interpmodule's set_call_level
setter accepted any int from Python into the same index.
tests/motion/heading gains an assertion that iscircle agrees with the motion
type, as a regression guard on the StateTag flag bits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
grandixximo
left a comment
grandixximo
left a comment
There was a problem hiding this comment.
Thanks, this is a clean design. Resolving the stack from the executing move's StateTag instead of the live interpreter is the right call, since the interpreter is typically at EOF long before motion finishes inside a sub. The aged-id detection in the ring and the all-or-nothing fallback on a failed mid-walk lookup are nice touches.
Two questions:
-
The PR body describes packing the level into bits 24-30 of
packed_flagsand building frames out ofsub_context[], but the code addsGM_FIELD_CALL_STACK_IDand a persistent node ring. Which is current? Worth syncing the body with the implementation. -
Nothing in-tree consumes this yet (axis, gmoccapy, qtvcp don't read even the existing
call_level). Do you have a UI follow-up in the pipeline, or is this landing as infrastructure for others to build on?
| cms->update((int *) &execState, 1); | ||
| cms->update((int *) &interpState, 1); | ||
| cms->update(callLevel); | ||
| for (int i = 0; i < EMC_MAX_CALL_STACK; i++) { |
There was a problem hiding this comment.
callLevel is serialized just above, so this loop could run to callLevel instead of EMC_MAX_CALL_STACK. On local shmem it's noise, but remote NML clients pay for 10 dead frames per cycle whenever the program is in main.
| if (settings->call_level >= INTERP_SUB_ROUTINE_LEVELS) { | ||
| // check before incrementing: leaving call_level past the end of | ||
| // sub_context[] would make unwind_call() index out of bounds | ||
| if (settings->call_level + 1 >= INTERP_SUB_ROUTINE_LEVELS) { |
There was a problem hiding this comment.
This fixes a real pre-existing out-of-bounds on sub_context[] (old code incremented past the end before erroring). Good catch. Could you mention it in the PR body so it doesn't hide inside the feature?
|
|
||
| print("{:6d} {}".format(nsamples, sample)) | ||
|
|
||
| # iscircle must agree with the motion type. This is a direct regression |
There was a problem hiding this comment.
This looks like it guards the old packed_flags approach, which this revision no longer uses. Still needed?
Gmoccapy, gladevcp and qtvcp use hal_glib to read the linuxcnc status. |
He demonstrated a Lathe-specific UI at the Stuttgart meetup which uses this. (Rather neatly) |
|
There seems to de a discrepancy between the max subroutine depth in the interpreter and the max. allowed call stack. |
|
The original version didn't use state tags, so with read ahead and more than one subroutine call you would get desychronised call stacks in the GUI. We fixed that at the Stuttgart meeting. IMO not showing the file that is currently executing is an anti-feature, I definitely want to see code of macros and even remaps, esp. in case of probing or tool changing routines and when single-stepping. |
|
We had some discussion how big to make the ring buffer. As the buffer is not living in shared memory (the state tag merely contains an opaque ID), it could be further refined to use more dynamic structures and keep in principle "infinite" call stacks. The structure in EMCSTAT should probably be increased to accomodate INTERP_SUB_ROUTINE_LEVELS frames. |
|
The code was generated by Claude IIRC, and I think 85mvh is not comfortable to write c++, so we will either have to fork this PR or get his Claude to impl the review changes. |
A case of "I fixed it but I don't know what I did or how I did it" is a very, very bad sign. |
I had a look at it, I'm somewhat familiar with the changed code, nothing was obviously wrong, contrary to the original version it also worked with multiple nested calls showing correct call stacks in @85vmh 's GUI, but it was also clear that some details like max call stack depth need some more thinking. |
I think the PR body pertains to the first (incorrect) version, that needs updating. |
|
Hi guys, I've built a conversational system that for some functions is relying on calling subroutines that can call other subroutines. With the current implementation, I could not represent that on the UI. 
I would like to have my changes integrated in master because it will be easier for me to continue my development without having to continuously rebase my fork with master. If these changes are not merged, my fork will go on in parallel with the changes that I need in order to obtain the features that I want in my app. |
Review fixes. EMC_TASK_STAT::update() sent all EMC_MAX_CALL_STACK frames on every cycle, so an NML client received ten empty frames whenever the program was running in the main file. It now sends callLevel frames. callLevel itself is sent just above the loop, so the reader already knows how many frames follow it. callLevel is also limited to the length of callStack[] before the loop uses it. When reading, it has just been taken out of the message and the loop uses it to index the array, so a damaged message would otherwise read past the end. Since the entries above callLevel are no longer sent, a GUI keeps whatever it last read in them. The comment on callStack[] now says that only the first callLevel entries are valid; the Python binding already returns only that many. emcTaskUpdate() still clears the rest, which now affects only task's own copy of the status, and the comment there says so. The comment above EMC_MAX_CALL_STACK said that its relation to INTERP_SUB_ROUTINE_LEVELS was "asserted in emctask.cc", and there was no such assertion. Add it to emctask.cc, the only file that includes both headers. Both constants are 10, so nothing changes size: the point is that they can no longer be changed apart from each other without the build failing. Remove the iscircle check added to tests/motion/heading/test-ui.py. It tested the StateTag flag bits that an earlier version of this work used, and the current one carries a node id in GM_FIELD_CALL_STACK_ID instead, so the check no longer covers anything this branch does. tests/motion/heading is now untouched by this branch. Raised in review by grandixximo (the frames sent and the heading test) and by BsAtHome and rmu75 (the array size). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 participants
Why
Task status carried only
callLevel, so a GUI could tell that a subroutine was runningbut not which one, nor where it was called from.
Worse,
callLevelwas sampled from the live interpreter atinterp_listdequeue time,so it described wherever the interpreter had read ahead to rather than the move the
machine was actually executing. The two are decoupled by up to
[TASK]INTERP_MAX_LENqueued canon commands plus the motion queue, so on a short program the interpreter is
typically at EOF long before motion finishes inside a sub.
How
Rather than reading the interpreter's current state, recover the stack that was active
when the executing move was interpreted:
call_stack_nodeholding the callsite (filename, line), the subroutine name, and the id of its caller — a linked
chain, so a whole stack is addressable by one integer. Nodes live in a fixed ring of
INTERP_CALL_STACK_NODES(16384, ~512 kB of non-realtime memory) and each stores itsown id, so a node whose slot has since been reused is detected on lookup instead of
being reported as some unrelated call.
enter_context()pushes,leave_context()pops,
unwind_call()resets to the root.write_state_tag()stamps the current node id into every block's StateTag asGM_FIELD_CALL_STACK_ID. It rides through segment merging and TP blending like theother tag fields.
InterpBasegainsresolve_call_stack_depth(node_id)andresolve_call_stack_frame(node_id, level, ...), with defaults that report an emptystack;
Interpimplements them by walking the node chain. A chain that cannot beresolved in full reports depth 0 rather than a partial stack against a truncated
depth.
emcTaskUpdate()resolves the tag of the move motion is executing and fillsEMC_TASK_STAT::callStack[], derivingcallLevelfrom the same id so depth and framesalways describe one point in the program. The stack is cleared once the interpreter
goes idle.
emcTaskExecute()no longer writescallLevel.EMC_TASK_STATgrowsEmcCallFrame {filename, subname, line}andcallStack[]of up toEMC_MAX_CALL_STACKframes. Frame i reads "at line L of file F we called subroutineS", with i == 0 being the call made from the main program. Only the frames in use are
serialised, so a program running in the main file costs nothing on the wire.
Exposed to Python as
stat.call_stack, a tuple of dicts withfilename,subnameandlinekeys.NML buffer size
The growth pushes
EMC_STATpast the 10240-byteemcStatusbuffer, which would makeNML::write()silently drop status and leave LinuxCNC looking hung to every GUI, withnothing failing at build time.
configs/common/{client,server}.nmlgo to 20480, andemcops.ccgains astatic_assertonsizeof(EMC_STAT)so the build trips before thatcan happen again.
A second static_assert in emctask.cc pins EMC_MAX_CALL_STACK to
INTERP_SUB_ROUTINE_LEVELS, so the status array and the interpreter's nesting limit
cannot drift apart.
Drive-by fix: out-of-bounds in
enter_context()Called out separately because it is a pre-existing bug, not part of the feature, and
worth applying on its own merit:
enter_context()incrementedcall_levelbefore testing it againstINTERP_SUB_ROUTINE_LEVELS, so on the error path it was left one past the end ofsub_context[]— whichunwind_call()then indexed. The test now happens before theincrement.
Tests
tests/interp/call-stackruns a program whose subroutines live in separate files andchecks that
stat.call_stackstill namesouter/innerwhile the machine is cuttinginside them — the point at which the live interpreter has already read to EOF, and
exactly the case the old
callLevelgot wrong.tests/interpandtests/motionpass in full (89 tests, 1 skipped —tests/interp/compile, already disabled on master).Note on an earlier revision
An earlier version of this PR packed the call level into bits 24–30 of the StateTag's
packed_flagsand rebuilt frames fromsub_context[]. That approach desynchronised thedisplayed stack from the executing move once read-ahead got more than one call deep, and
it was replaced at the Stuttgart meetup by the node-id design described above. This
description previously still described that first version; it has been rewritten to match
the code. Thanks to @rmu75 for spotting the mismatch.