[GRASS5] NVIZ and threaded TCL 8.4 working

Hi,

The subject is indeed correct:

http://www.shockfamily.net/cedric/grass/nviztcl84threaded.png

My tcl and tk 8.4 binaries are Ubuntu, which are probably pretty similar to
debian's.

Many of the nviz c source files call Tcl_Merge to make return strings, and
assign them to interp->result where interp is the TCL interpreter like this:

interp->result = Tcl_Merge(3, list);

Then they set the freeProc ("function" to call to free the memory) to be free
(glibc). Tcl_Merge allocates its memory with Tcl_Alloc, not with malloc.
Before 8.4 Tcl_Alloc just happened to be malloc, but it's not anymore. The
correct thing to do is to set the freeProc to be TCL_DYNAMIC, which will be
TCL's opposite of Tcl_Alloc, like this:

interp->freeProc = TCL_DYNAMIC;

I replaced all of the references to glibc free as freeProc (only related to
results that use Tcl_Alloc, which turns out to be all of them) with
TCL_DYNAMIC. I also found one case of sprintf-ing to interp->result without
allocating memory and replaced it with the appropriate (Tcl_SetResult?) code.

I am not including a patch, as the number of differences between my current
codebase and CVS is ridiculous to try to deal with by editing cvs diff -u -p
patch files by hand.

--Cedric Shock

A quick correction,

... I also found one case of sprintf-ing to interp->result without
allocating memory and replaced it with the appropriate (Tcl_SetResult?)
code.

grepping the code it looks like there are at least 30 more of these. They
aren't segfaults because TCL is already prepared for them (it's even
documented). result is initialized pointing to space for about 200
(TCL_RESULT_SIZE) characters. I'd guess that sprintf-ing to result is much
more future proof than sprintfing to a local variable. I'll undo that part of
my changes.

--Cedric Shock

Cedric Shock wrote:

My tcl and tk 8.4 binaries are Ubuntu, which are probably pretty similar to
debian's.

Many of the nviz c source files call Tcl_Merge to make return strings, and
assign them to interp->result where interp is the TCL interpreter like this:

interp->result = Tcl_Merge(3, list);

Then they set the freeProc ("function" to call to free the memory) to be free
(glibc). Tcl_Merge allocates its memory with Tcl_Alloc, not with malloc.
Before 8.4 Tcl_Alloc just happened to be malloc, but it's not anymore. The
correct thing to do is to set the freeProc to be TCL_DYNAMIC, which will be
TCL's opposite of Tcl_Alloc, like this:

interp->freeProc = TCL_DYNAMIC;

Will this also work in 8.3?

--
Glynn Clements <glynn@gclements.plus.com>

Glynn,

Will this also work in 8.3?

It's documented that way back to 8.0:

http://www.tcl.tk/man/tcl8.0/TclLib/Interp.htm

Plus I ran it in 8.3 while testing amongst running it in 8.4.

What I don't understand is why grass 6 nvis runs fine on my computer with 8.4
and threads.

--Cedric

Oops,

What I don't understand is why grass 6 nvis [sic]
runs fine on my computer with
8.4 and threads.

Of course that's because the Ubuntu packages were linked against libtcl8.3. I
configured my CVS builds to link against 8.4. I had just never tried to run
nviz from CVS before trying today, which is also why it looked so alien to
me.

Plus I ran it in 8.3 while testing amongst running it in 8.4.

As a result this statement is false, all my tests were linked against 8.4.

--Cedric

> Many of the nviz c source files call Tcl_Merge to make return
> strings, and assign them to interp->result where interp is the TCL
> interpreter like this:
>
> interp->result = Tcl_Merge(3, list);
>
> Then they set the freeProc ("function" to call to free the memory)
> to be free (glibc). Tcl_Merge allocates its memory with Tcl_Alloc,
> not with malloc. Before 8.4 Tcl_Alloc just happened to be malloc,
> but it's not anymore. The correct thing to do is to set the
> freeProc to be TCL_DYNAMIC, which will be TCL's opposite of
> Tcl_Alloc, like this:
>
> interp->freeProc = TCL_DYNAMIC;

Will this also work in 8.3?

fwiw, on Debian/stable with tcl8.3-dev, tcl8.4-dev, and tk8.4-dev installed:

$ grep -r TCL_DYNAMIC /usr/include/tcl8.* | cut -f4- -d'/'
tcl8.3/tcl-private/generic/tcl.h: * statically allocated. TCL_DYNAMIC means
tcl8.3/tcl-private/generic/tcl.h:#define TCL_DYNAMIC ((Tcl_FreeProc *) 3)
tcl8.3/tcl-private/generic/tclInt.h: * allocated. TCL_DYNAMIC means string
tcl8.3/tcl.h: * statically allocated. TCL_DYNAMIC means
tcl8.3/tcl.h:#define TCL_DYNAMIC ((Tcl_FreeProc *) 3)
tcl8.4/tcl-private/generic/tcl.h: * statically allocated. TCL_DYNAMIC means
tcl8.4/tcl-private/generic/tcl.h:#define TCL_DYNAMIC ((Tcl_FreeProc *) 3)
tcl8.4/tcl-private/generic/tclInt.h: * allocated. TCL_DYNAMIC means string
tcl8.4/tcl.h: * statically allocated. TCL_DYNAMIC means
tcl8.4/tcl.h:#define TCL_DYNAMIC ((Tcl_FreeProc *) 3)

Today's gold star goes to Cedric.

Hamish