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

[Donal F. <don...@ma...>]

I cannot comment on the complexity of fixing the BCE, don’t know enough but to be on the safer side, I would have favored an approach that fixed the BCE before changing the INT_MAX limit to UINT_MAX. Conceptually, check immediate operands against BCE_MAX, set to INT_MAX for now and changed to UINT_MAX when the BCE was fixed.
I can comment on that now from a position of a little authority: the tebc-opnd-types branch fixes the issue there (and passes tests at least on Windows/Msys). That branch is independent of the alterations in the branch for TIP 720, though the two interact a bit (with there being new opcodes, that's unsurprising).

In the process, the branch does away with the generic opnd variable; it carried too little semantics and was conflating different things; things like variable indices, aux table indices and jump offsets are now their own things. Note that mostly the actual variables that are used are of type Tcl_Size even though the values in the bytecode are naturally unsigned because otherwise we get weird crashes (due to negation being needed to access things relative to the stack top). I've checked that the operands are read correctly from the bytecode stream so that they're meaningful; there were a few places where things that were semantically unsigned were being read as signed, and nobody had ever checked.

I didn't want this particular fix (which I thought needed anyway) to be mixed with the improvements to bytecode issuing.

Donal.
________________________________
From: apn...@ya... on behalf of apn...@ya...
Sent: Friday, May 16, 2025 03:38
To: Donal Fellows; 'Tcl Core'
Subject: RE: TIP 720: Updated Tcl Bytecode Opcodes CFV (NOTE: NO DELAY)


Donal,



I think we are in agreement that the byte code engine has issues with type correctness and mixed signed/unsigned usage. Additionally, it does not support 8 byte immediate values in the byte stream (which I would not classify as a bug but a RFE). The former has implications for values in the range (INT_MAX, UINT_MAX]. The latter has implications for (UINT_MAX, TCL_SIZE_MAX].



9.0.{0,1} dealt with both these by checking values against INT_MAX  in the compile phase and punting if necessary. This ensures correct operation of the byte code engine at the cost of slower execution via invokeStk (I think?).



By changing the limit to UINT_MAX, values in (INT_MAX, UINT_MAX] can now also be passed within the byte stream (though not UINT_MAX:TCL_SIZE_MAX). However, now the byte code engine will not operate correctly (potentially and admittedly in very rare cases) *unless its type-correctness issues are fixed*. If I understood you correctly, your view is that fixing the BCE is not within the purview of your branch but a task for some later branch.



I cannot comment on the complexity of fixing the BCE, don’t know enough but to be on the safer side, I would have favored an approach that fixed the BCE before changing the INT_MAX limit to UINT_MAX. Conceptually, check immediate operands against BCE_MAX, set to INT_MAX for now and changed to UINT_MAX when the BCE was fixed.



But that’s only because of *my* preference for not introducing regression into the main branch.



/Ashok





From: Donal Fellows <don...@ma...>
Sent: Thursday, May 15, 2025 4:27 PM
To: apn...@ya...; 'Tcl Core' <tcl...@li...>
Subject: Re: TIP 720: Updated Tcl Bytecode Opcodes CFV (NOTE: NO DELAY)



In this case, the branch is effectively starting from the point where the conversion to Tcl_Size has already taken place internally in the compiler; it's just renaming some of them so that I could better see what was going on. The aim was that I'd be able to look up the definition of a variable being used as, say, a jump offset and see that it isn't also being used for variable indices and/or aux data indices.



The range checks are for ensuring that the value read by TclGetUInt4AtPtr (widespread in the bytecode engine) is matched by the range checks in the compiler (where the values are [0,UINT_MAX] plus -1 for no index, which fits Tcl_Size nicely). Fixing the opnd variable usage within the bytecode engine is something for another branch I think; there's a lot of mixed usage where we ought to do something a bit more type-correct. But that's a bug already in the bytecode engine so I'm not condemning this branch to resolve it!



Donal.

________________________________

From: apn...@ya...<mailto:apn...@ya...> on behalf of apn...@ya...<mailto:apn...@ya...>
Sent: Wednesday, May 14, 2025 17:31
To: Donal Fellows; 'Tcl Core'
Subject: RE: TIP 720: Updated Tcl Bytecode Opcodes CFV (NOTE: NO DELAY)



I’m really not worried about LVT indices, it just happened to be the first (and only) range check that showed up in the diffs. The bigger issue is that these range checks appear in lots of places and any of them could have similar repercussions. I feel changing the limit to UINT_MAX from INT_MAX has limited value and not worth the risk of fiddling with TEBCResume to *selectively* split the signed and unsigned use cases. And as discussed ad naseum in the Tcl_Size discussion, modifying types from signed->unsigned is not to be taken lightly.