[m-rev.] Fwd: for review: fix obsolete tcl-tk code in extras/graphics/mercury_tcltk

[Fabrice Nicol]

Yes, Julien, you are (theoretically) right. You will find attached a new 
patch that takes your review into account.

Practically yet, for reasons I still poorly understand, using the setter 
functions (Tcl_SetResult or Tcl_SetObjResult) as you indicate in your 
review causes a runtime crash on using the 'calc' executable built with 
the tcltk library code fixed along these lines, __unless the TCL_STATIC 
memory management flag__ is set in the third argument of the setters.

There may be some C-memory management issue, as the TCL-TK setter 
functions perform C-style memory allocation under the hood. I guess that 
the dynamic version (TCL_DYNAMIC, which is often the preferred option) 
is somehow incompatible with the Mercury GC, and I am not quite sure 
that using TCL_STATIC releases all tcl-tk-allocated memory on exit.

Oddly enough, 'calc' works just fine (for all arithmetic operations) if 
the getter function Tcl_GetStringResult is used instead of the setters, 
as in my earlier proposed patch. This may be caused by the fact that the 
underlying 'interp' structures are not strictly write-protected (this is 
not C++ code), which may explain why the getter function is optimized 
away by the compiler as an alias for the 'result' field of 'interp', as 
in prior code. This may be a case of efficient, yet unclean coding.

Fabrice
>
> Hi Fabrice,
>
> On Wed, 5 Jan 2022, Fabrice Nicol wrote:
>
>> As a modest proposal for review by anyone.
>>
>> ----------------------------------------------------------
>>
>> Bug issue (fixed): building libmercury_tcltk crashes with current 
>> tcl-tk releases (8.6+).
>>
>> As a consequence, samples/calc cannot be built either.
>
> ...
>
>> diff --git a/extras/graphics/mercury_tcltk/mtcltk.m 
>> b/extras/graphics/mercury_tcltk/mtcltk.m
>> index 483f89c82..238a26cdc 100644
>> --- a/extras/graphics/mercury_tcltk/mtcltk.m
>> +++ b/extras/graphics/mercury_tcltk/mtcltk.m
>> @@ -224,7 +224,7 @@ Tcl_AppInit(Tcl_Interp *interp)
>>              MR_fatal_error(""Tcl_Eval returned neither TCL_OK or 
>> TCL_ERROR"");
>>      }
>>
>> -    Result = mtcltk_strdup(Interp->result);
>> +    Result = mtcltk_strdup(Tcl_GetStringResult(Interp));
>>  ").
>>
>>  :- pragma foreign_code("C", "
>> @@ -284,8 +284,9 @@ mtcltk_do_callback(ClientData clientData, 
>> Tcl_Interp *interp,
>>              (MR_Word) args);
>>      }
>>
>> +    char* s = Tcl_GetStringResult(interp);
>>      mtcltk_call_mercury_closure((MR_Word) clientData, interp,
>> -        args, &status, &interp->result);
>> +        args, &status, &s);
>
> That second part doesn't look correct.
> The call to mtcltk_call_mercury_closure() sets 's' to point to a string
> (i.e. it's an output argument) and this code should _set_ interp->result
> to that string.  So rather than calling Tcl_GetStringResults() before
> the call, I think there should be a call to Tcl_SetResult() or
> Tcl_SetObjResult() somewhere after it.
>
> Julien.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-mercury_tcltk-obsolete-result-fields.patch
Type: text/x-patch
Size: 1589 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20220105/be38325e/attachment.bin>