Tcl Source Code

Of course, this could be fixed the same way as [incr Tcl Bug #1725219]
 <https://sourceforge.net/tracker/index.php?func=detail&aid=1725219&group_id=13244&atid=313244>

But, much easier and much less hacky is to just
add dummy unused fields to the
Tcl_CallFrame and CallFrame structures.

Here is the patch, for Tcl 8.5 and 8.4.

For Tcl 8.6 nothing changes. But when
compiling Itcl 3.x, just some extra unused
stack space is reserved, so whenever the
same Itcl built is run under Tcl 8.6 this
space can be used at will.

allow_comments - 1

Fixed in 8.5, 8.4 and 8.3 branch.

If soneone asks: 8.3???? does someone still use that?
Answer: Itcl 3.4b1 still works fine with Tcl 8.3. So at least
I had to verify that this patch works there too. No-one
is planning to release a new Tcl 8.3 version, it's officially
dead, but if someone (like me) wants to revive it
in his own environment, the 8.3 branch still exists
and works.

Final suggestion: The only reason I added dummy fields to CallFrame in tclInt.h, was
that tclBasic.c demands it. However, this bug shows that in fact Tcl_CallFrame
cannot be smaller than CallFrame, so that's the requirement that should
be checked in tclBasic. Here is the patch.

This allows me to revert my changes to tclInt.h, so the Tcl binary produced is
exactly as before. The only intention of this patch is to tell extenions:
"please allocate a little bit more for your Tcl_CallFrame, because future
Tcl versions might need it".

I'll revert my changes to tclInt.h, and apply the patch to tclBasic.c this
weekend. Jeff, Don, is that safe enough?

File Added - 380572: tclBasic.c.diff

>Tcl never allocates a (Tcl_)?CallFrame

I should have added there: 
   "on the stack"
Tcl allocates CallFrames in two places
(tclBasic.c:610 and tclNamesp.c:413),
that would be the only places in the
binaries that are affected by this patch.

But even in those 2 places, Tcl only
allocates those in order to use the
Tcl_(Push|Pop)CallFrame API the
same way as extensions do. So,
still, all arguments hold.

>I'm sorry to say that, but this is simply not true. Itcl
>compiled against 8.5.9 just runs fine when running
>in 8.5.x (x<9). 
Let me add why, and I'l rest my case.
Tcl never allocates a (Tcl_)?CallFrame, it's the
extension that is supposed to do that. This means
that this patch does not even change a single
byte inside Tcl binaries at all! Tcl binaries with
or without this patch are binary equal, so
sure they are compatible.

The ONLY effect this patch has, is that
Itcl allocates a little more space for the
Tcl_CallFrame that is given to Tcl. The
change to tclInt.h was not even
necessary, although sure it will raise
questions when Tcl_CallFrame and
CallFrame are not equal in size any more.

I rest my case.

> This means that binaries built using the 8.5.9 tcl.h
> header file will allocate larger Tcl_CallFrame structs.
> This means those binaries will not function when [load]ed
> or otherwise matched up at runtime with a Tcl library
> from release 8.5.x where x < 9.

I'm sorry to say that, but this is simply not true. Itcl
compiled against 8.5.9 just runs fine when running
in 8.5.x (x<9). 

> if Itcl
> hasn't bothered to stay compatible with Tcl 8.5 -- now
> several years old -- that's a problem for Itcl to solve.
That's exactly what [incr Tcl Bug #1725219] is about,
which was handled by jeff, so I would like to hear
his view before going on.

In addition, it's of quite questionable value to add aids
to assist the migration of Itcl 3.* from Tcl 8.5 to Tcl 8.6
since Tcl 8.6 is going to come with Itcl 4.

On a less technical, more political note, if Itcl
hasn't bothered to stay compatible with Tcl 8.5 -- now
several years old -- that's a problem for Itcl to solve.
Not something for Tcl to twist itself into accomodating.

We spent too many years with crazy Aunt Millie in
init.tcl from pursuing this reasoning in the past.
Enough.  If Itcl is broken, then fix it, or discard it.

Your commit to core-8-5-branch adds a field "dummy13"
to the end of the public definition of the Tcl_CallFrame
struct.

This means that binaries built using the 8.5.9 tcl.h
header file will allocate larger Tcl_CallFrame structs.
This means those binaries will not function when [load]ed
or otherwise matched up at runtime with a Tcl library
from release 8.5.x where x < 9.

This kind of incompatibility is one we have customarily
reserved for new minor releases, not patchlevels.  At
a minimum, reacting to such a policy change means
that extensions have to rethink their Tcl_InitStubs()
and [package require Tcl] habits so that they begin
checking for compatible Tcl libraries all the way to
the patchlevel detail.

Maybe a case could be made that that's a desirable
way to go, but it should be something proposed and
consideredand discussed, with some feedback from some actual
extension authors, not something just dropped in
over the weekend.

I say revert.

See the following comment in tcl.h:
==========================================================================
A call frame is initialized and pushed using Tcl_PushCallFrame and popped
using Tcl_PopCallFrame. Storage for a Tcl_CallFrame must be provided by the
Tcl_PushCallFrame caller, and callers typically allocate them on the C call
stack for efficiency. For this reason, Tcl_CallFrame is defined as a
structure and not as an opaque token. However, most Tcl_CallFrame fields
are hidden since applications should not access them directly; others are
declared as "dummyX".
==========================================================================

Example extension code:

 Tcl_CallFrame frame;
 int somevar;

 Tcl_PushCallFrame(interp, &frame, ....);
 ....
 Tcl_PopCallFrame(interp);

The functions Tcl_PushCallFrame and Tcl_PopCallFrame are functions
that look like public, but in fact they are in the private stub
table. The Tcl_CallFrame structure looks like a public structure,
but in fact is was never documented. As far as I know, the only
extension using it is Itcl. So, is it really public?....

Just because Tcl 8.5 added two fields to the Tcl_CallFrame structure
this caused [incr Tcl Bug #1725219]. Tcl 8.6 did it again with one
additional field. So, yes, you are right that "Adjusting the size
of any public structure is a recipe for problems". That's exactly
what caused [incr Tcl Bug #1725219]. The problem is caused by
overwriting variables that come immediately afther the Tcl_CallFrame
structure on the stack, that that's exactly what this patch is
not doing.

So, what happens if the above example code is compiled against
Tcl 8.4.19? The "frame" variable is allocated on the stack, and
is used to hold the old callframe between the Tcl_PushCallFrame
and Tcl_PopCallFrame calls. Now we are running it in Tcl 8.5 or
Tcl 8.6. The Tcl_CallFrame structure got some extra
fields, so the result is that the "frame" variable is not
big enough any more to hold the information stored by Tcl.
The effect will be that the "somevar" variable is overwritten.

So, now we run it in Tcl 8.4.20 (with this patch). Yes, the
Tcl_CallFrame got some extra fields. But Tcl never reads or
writes those dummy fields, so the "somevar" variable will
never be overwritten.

Now what happens if we compile the above sample code using
Tcl 8.4.20 (with this patch). The effect is that the "frame"
variable gets some extra space, and the the "somevar" variable
is moved farther away on the stack. When running it on
Tcl 8.4.19, this extra space is not used. But when running
it on Tcl 8.5 or Tcl 8.6, this space will be used by the
Tcl_PushCallFrame function! But this patch prevents that
the "somevar" is overwritten, so the bug is solved.

This is fully compatible, because the added fields are unused! The only effect is that in newly compiled code, a little more space is allocated than necessary. But this space is necessary when running under Tcl 8.6. It's exactly the same as (but better than)  the hack used in Itcl 3.4, only it can be used by all extensions using (Tcl_)?CallFrame.

Please provide an example where this causes a problem, before suggesting that I broke any compatibility. There isn't!

allow_comments - 0

This seems like a misguided patch.  Adjusting the size of any public structure is a recipe for problems.  If this has been used in existing code, you may well have just broken compatibility for 8.4 and 8.5.  This doesn't seem like a safe enough approach.

allow_comments - 1

fixed in Tcl 8.5 and 8.4 branch

File Added - 380337: 3030870.patch