Re: [TCLCORE] TIP 720: Updated Tcl Bytecode Opcodes CFV (NOTE: NO DELAY)

[Steve L. <st...@di...>]

I’m conflicted on this one.   The intent is good and (as Donal said in his original email) it arguable doesn’t need a TIP. On the other hand I don’t understand the details of the implementation and don’t have the time nor energy to come up to speed on the bytecode engine.  Having said all of that, I have followed the discussion with interest and wouldn’t stand in the way.

So do I vote PRESENT or YES?    If this was a user-visible or breaking change I’d make it PRESENT but on this occasion I’ll make it ....

TIP #720:  YES

-- Steve
On 19 May 2025 at 10:55 AM +0800, apnmbx-public--- via Tcl-Core <tcl...@li...>, wrote:
> Donal,
>
> Thanks for the detailed explanations.
>
> Regarding the IDEOGRAPHICSPACE bug, IsEmptyToken does not appear in any other branch than no-variable-width-instruction-issue so must have been introduced there. And fossil blame shows you as the author 😊 Perhaps you might be confusing the IsEmptyToken function hosting the bug with Tcl_IsEmpty? It’s all trivial to fix as long as we’re all on the same page as to what constitutes whitespace.
>
> One other comment, not directly pertaining to 720 but related, regarding the other branch no-var-width-plus-opnd-types fixing TEBCResume types. I think the use of Tcl_Size fixes type mismatches for 64-bit archs but not 32-bit. For example, fragments like
>
> Tcl_Size numArgs;
> ...
> numArgs = TclGetUInt4AtPtr(pc + 1);
> objResultPtr = OBJ_AT_DEPTH(numArgs);
>
> will work on 64-bit arch but run into the same unsigned->signed offset issues on 32-bit architectures. Again, to reiterate, not that it currently matters because of the script and argument length limits of INT_MAX currently in place but will probably matter with TIP 626 approval.
>
> /Ashok
>
> From: Donal Fellows <don...@ma...>
> Sent: Monday, May 19, 2025 2:46 AM
> To: apn...@ya...; 'Tcl Core' <tcl...@li...>
> Subject: Re: [TCLCORE] TIP 720: Updated Tcl Bytecode Opcodes CFV (NOTE: NO DELAY)
>
> Re the compilation errors, I've no idea how they ended up in there, but they're fixed now. If there's still MSVC errors, please let me know what the error message is; I'm having problems triggering it here. (I might just make that code path depend on recent enough standard C if I can't figure out any other alternative; this will be fixed prior to merging.)
>
> Re the change to use IsEmptyToken, it's the intention that an empty body results in nothing happening, and that if IDEOGRAPHIC SPACE is now considered a space for the purpose of that function, then the code is now behaving correctly. [dict with] is very happy to delegate that decision. If IsEmptyToken is reporting tokens as empty when they could be compiled to code with side effects, that's definitely a bug... but not with the callers of IsEmptyToken. I'm not going to probe that further; I didn't write IsEmptyToken, but I need something that says "this token definitely doesn't generate code and so definitely doesn't generate errors".
>
> Literal sharing is one of those things that I think we may have been doing a bit too much of, and which has caused unnecessary representation churn. OTOH, I'm not strongly invested in either sharing or not sharing those objects for the most part. My usage is on the basis of it being a convenient API; I have a Tcl_Obj and I want to arrange for it (or something remarkably similar) to get pushed onto the stack by the bytecode engine at that point.
> I believe I checked the cases where I switched the code over to bounce the refcount; they're supposed to be places where the called code may take a reference or may not (typically on error) but definitely won't be decrementing the reference count. (It's the functions with an internal Tcl_DecrRefCount that you need to watch out for.)
> The switch to use Tcl_ListObjReplace was indeed deliberate. It's so that appending a zero-length list works correctly, and that's necessary for the short lappend-expansion branch to work (which makes [lappend] do the right thing with {*}). Previously, [lappend foo] was an ugly special case precisely because the opcodes worked differently from the non-compiled [lappend]. So. Many. Edge. Cases.
> Remove the clause, run append.test and appendComp.test (I forget which one) and see something very strange with some tests. I think it might be append-7.4 that triggers it, but I'm not sure as I've not poked at it for a few weeks (that have been very busy ones for me). The code before this branch (https://core.tcl-lang.org/tcl/file?name=generic/tclCompCmdsGR.c&ci=22313fe35a5e2956&ln=856,860) is also odd in this area; in a sane world, the first condition there would be be numWords<2 and the second would be without looking at the envPtr->procPtr at all (at that point; it should just be a concern for picking how to access a variable, such as in https://core.tcl-lang.org/tcl/file?name=generic/tclCompCmdsGR.c&ci=22313fe35a5e2956&ln=897,909 and https://core.tcl-lang.org/tcl/file?name=generic/tclCompCmdsGR.c&ci=22313fe35a5e2956&ln=923,935).
> I'm generally supportive; the deprecated opcodes should be fairly easy to identify and remove with things as they stand. We do not generate any of them at this point, but there's still code that knows how to work with them (and execute them, of course). If backward compatibility is truly undesired, we can also remove the binding of the remaining opcodes to their current values. But technically out of scope right now. 😄
>
> Donal.
>
> From: apn...@ya... <apn...@ya...> on behalf of apn...@ya... <apn...@ya...>
> Sent: Saturday, May 17, 2025 17:37
> To: Donal Fellows <don...@ma...>; 'Tcl Core' <tcl...@li...>
> Subject: RE: [TCLCORE] TIP 720: Updated Tcl Bytecode Opcodes CFV (NOTE: NO DELAY)
>
> Ok, having read the main components in tclComp*.c and tclExecute.c, I think I'm good to go on TIP 720. The code generation is much more readable making future development easier.
>
> Even the minor beef I had with the INT_MAX/UINT_MAX limits is not an issue currently as two separate INT_MAX limits, one on the length of a script and the other on the number of arguments, make the point moot. The (INT_MAX..UINT_MAX] case will never actually occur afaict. At least until TIP 626 at which point it becomes Jan's problem :-)
>
> There are some compilation errors still on Windows as mentioned earlier and also on Unix with --enable-symbols=all (works without that).
>
> One little buglet, not actually related to TIP 720 but related to the changes in [dict with] arising from modification to using Unicode aware IsEmptyToken. Didn't check if [dict update] also had the same issue.
>
> Tcl 9.0.1
> % info patch
> 9.0.1
> % writeFile space.tcl "proc \u3000 {} {puts IDEOGRAPHICSPACE}\n; set d \[dict create a 0];dict with d {\u3000}"
> % source space.tcl
> IDEOGRAPHICSPACE
>
> TIP-720
> % info patch
> 9.1a0
> % writeFile space.tcl "proc \u3000 {} {puts IDEOGRAPHICSPACE}\n; set d \[dict create a 0];dict with d {\u3000}"
> % source space.tcl
> % (no output as the dict with script block is treated as an empty script)
>
> Some more questions, as much for my own edification, as anything else…
>
> (1) Code sequences of the form
>
> Tcl_Size len;
> const char *bytes = TclGetStringFromObj(objPtr, &len);
> PushLiteral(envPtr, bytes, len)
>
> have been replaced by
>
> PUSH_OBJ(objPtr)
>
> Whereas PushLiteral resolves to TclRegisterLiteral, PUSH_OBJ resolves to TclAddLiteralObj. The former adds the value to the global literal array, but the latter does not. Is this difference intentional? Since the literal is going to be in the local array anyways, doesn't it make sense to have it globally available as well for memory sharing as in 9.0.0?
>
> (2) I also see sequences like
>
> TclNewObj(objPtr);
> Tcl_IncrRefCount(objPtr);
> SomeFunction(...objPtr...);
> Tcl_DecrRefCount(objPtr);
>
> replaced by
>
> TclNewObj(objPtr);
> SomeFunction(...objPtr...);
> Tcl_BounceRefCount(objPtr);
>
> but the semantics of the above sequences are not the same as the latter does not guarantee objPtr is still accessible on return from SomeFunction. I have not looked into every such "SomeFunction" but presumably, they do not manipulate (e.g. incr+decr) the ref count of objPtr. Still, the original sequence seems safer unless all SomeFunction’s in this context are known to be safe in this respect.
>
> (3) In the TEBCResume function, the under label lappendListPtr:, the call to TclListObjAppendElements has been replaced by Tcl_ListObjReplace. Why so? The latter is more general and therefore has to do a little more work to append. Clearly the change is intentional so I am curious as to the reason.
>
> (4) There is the following comment somewhere in the code which I think you just added:
>
>     /*
>      * The weird cluster of bugs around INST_LAPPEND_STK without a LVT ought
>      * to be sorted out. INST_LAPPEND_LIST_STK does the right thing.
>      */"
> What does this cluster of bugs refer to ? Are they recorded somewhere? I've worked on lappend and have not seen or noticed any. This is just my curiosity as to what I might have missed.
>
> (5) As an RFE, not necessarily part of TIP 720, I would be tempted to expunge the deprecated opcodes from TEBC as well (not just disabled via macros). It would significantly reduce and simplify TEBCresume. I understand the potential incompatibility with respect to tbcload and possible the Tcl debugger/prodebug. However, I do not see any need to maintain 8.6 tbcload compat given we don't even maintain 8.6 compat at script or C API level. This is the time to eliminate that code (before people depend on tbcloaded 9.0 applications) else we would have to wait for 10.0. (As an aside, does tbcload work with Tcl 9? If not, is anyone even working on it?)
>
> In summary, all looks good as far as I am concerned once the compiler errors with --enable-symbols=all are fixed.
>
> /Ashok
>
>
>
> From: Donal Fellows <don...@ma...>
> Sent: Friday, May 9, 2025 5:47 PM
> To: Tcl Core <tcl...@li...>
> Subject: [TCLCORE] TIP 720: Updated Tcl Bytecode Opcodes CFV (NOTE: NO DELAY)
>
> As promised, I've done a TIP for the updates to the bytecode compiler. It's largely there to document the notable features of a substantial changeset.
>
> https://core.tcl-lang.org/tips/doc/trunk/tip/720.md [core.tcl-lang.org]
>
> It covers the essentials; of particular note is that no true public API is affected. That said, tcl::unsupported::assemble is affected, but via enhancement (it gets new opcodes), and tdkcompiler/tbcload will definitely need more work due to the new auxiliary structure type (for a numeric jump table).
>
> Given the lack of public surface changes - if you're not messing with compilation guts, you shouldn't need to care - I'm going to call a vote on this TIP immediately.
>
> Votes to here by [clock format 1747393200] (Fri May 16 12:00:00 BST 2025) in the usual form.
>
> TIP 720: YES
>
> Donal.
>
>
> _______________________________________________
> Tcl-Core mailing list
> Tcl...@li...
> https://lists.sourceforge.net/lists/listinfo/tcl-core