Tcl Source Code

View Ticket
Login
Ticket UUID: 4663e0636f7a24b9363e67c7a3dd25e9e495be17
Title: TIP 538 and Tcl extensions: 'tommath.h' file not found
Type: Bug Version: core-8-branch
Submitter: mr_calvin Created on: 2020-05-17 23:00:45
Subsystem: 56. LibTomMath Assigned To: jan.nijtmans
Priority: 5 Medium Severity: Severe
Status: Closed Last Modified: 2020-05-31 20:38:16
Resolution: Fixed Closed By: jan.nijtmans
    Closed on: 2020-05-31 20:38:16
Description:
We noticed that since TIP 538 became integrated into core-8-branch, our NSF builds started failing because tommath.h cannot be picked up anymore. See https://travis-ci.com/github/nm-wu/nsf/jobs/334291146#L717

> In file included from ./generic/nsf.c:16655:
>
> /home/travis/tcl/generic/tclTomMath.h:11:10: fatal error: 'tommath.h' file not found
>
> #include "tommath.h"

NSF includes tclTomMath.h explicitly, to access mp_int at one spot.

TIP 538 advertises that Tcl extensions should be able to include either tommath.h directly, or tclTomMath.h (as NSF does). 

How should an extension properly include tclTomMath.h (which unconditionally includes tommath.h) when built against a Tcl core not using a systemwide libtommath? Apparently, tommath.h is not provided to extensions that way (anymore)?

Thank you!
User Comments: jan.nijtmans added on 2020-05-31 20:37:47: (text/x-fossil-wiki)
> do you plan to merge the "fix" any time soon? just curious.

I just wanted to wait for your confirmation that it helps. Got that now (Thanks!), so it's merged now.

> I wondered: It would be nice to add a hint reflecting TCL_WITH_EXTERNAL_TOMMATH to tcl::pkgconfig (to figure out the LTM nature of Tcl's from within a script), wouldn't it?

The real way to discover this is the TCL_DEFS macro in tclConfig.sh. It works, but it is hardly used by any extension. The TEA tcl.m4 has a macro to discover which libraries Tcl is linked with. libz and libtommath were never taken into account, but I added that now.

> Why did TIP 538 propose a --with-system-libtommath switch ...
Well, I didn't really like that either: If libtommath is available, I see no reason to use the internal libtommath nevertheless. But it still appears people want that. I think that - as soon as Tcl 8.7 gets final - more distro's will start providing libtommath as standard packages, the main ones already have it. So this will become less and less a problem.

Thanks for all the feedback. I'll keep it like it is now for a while, let's see what more feedback arises when the next Tcl 8.7 alpha is out.

mr_calvin added on 2020-05-31 17:48:51:
> Well, I hope you meant LTM 1.2, since LTM 2.0 is not released yet and far from final, not even alpha....

Yeah, my bad, I meant 1.2, certainly.

> I would recommend the homebrew version on MacOS, it works for me out-of-the-box

thanks for the hint. actually, the standard makefile of ltm 1.2 works for me, but it is difficult to redirect to another installation directory. homebrew is not attempting such a redirection (as opposed to macports), therefore, it works out of the box, as you write.

do you plan to merge the "fix" any time soon? just curious.

jan.nijtmans added on 2020-05-31 14:54:52: (text/x-fossil-wiki)
> Took me a while to get LTM 2.0 installed and recognized by Tcl under macOS, no homebrew

Well, I hope you meant LTM 1.2, since LTM 2.0 is not released yet and far from final, not even alpha.... I would recommend the homebrew version on MacOS, it works for me out-of-the-box

mr_calvin added on 2020-05-31 09:11:05:
Hi Jan, 

Thanks again for your help and sry for getting back to it with much of a delay:

* I use

#if TCL_MAJOR_VERSION > 8 || TCL_MINOR_VERSION > 6
#define TCL_NO_TOMMATH_H 1
#endif
#include <tclTomMath.h>

and tested it against a Tcl at [1baf516ed4f18e08]  with embedded LTM and with a system-wide LTM. All looks good. (Took me a while to get LTM 2.0 installed and recognized by Tcl under macOS, no homebrew.)

* I wondered: It would be nice to add a hint reflecting TCL_WITH_EXTERNAL_TOMMATH to tcl::pkgconfig (to figure out the LTM nature of Tcl's from within a script), wouldn't it?

* Why did TIP 538 propose a --with-system-libtommath switch, rather than a --with-libtommath=/optional/path/to/ltm/installation ... ? This would be useful if ltm is not provided in a standard location (/opt/local etc.) and could still default to standard paths, if omitted? This is more in line with what others (e.g. tDOM) do in similar situations (system-wide expat).

* A TEA_TOMMATH macro would be still appreciated, because this way not every TEA compliant extension needs to be manually fixed to add includes and link-time flags on its own (iff build against a Tcl with external LTM).

Thx, Stefan

mr_calvin added on 2020-05-20 17:53:25:
I was confused, in this case, mp_clear is provided as a macro pointing into the stub table, I just realized.

mr_calvin added on 2020-05-20 17:38:02:
Thanks for this move, I will give it a try tonight!

Till then, one question: To avoid the dependency on mp_clear (which is not exposed including tommath.h) , and rather than using a throw-away Tcl_Obj as you suggested, I would like to maintain one Tcl_Obj and keep invalidating its intrep: Will TclFreeIntRep() also trigger mp_clear on a bignum Tcl_Obj, reliably?

Thank you!

jan.nijtmans added on 2020-05-19 15:51:30: (text/x-fossil-wiki)
@mr_calvin:  So, how about [1baf516ed4f18e08] ??   :-)

mr_calvin added on 2020-05-19 10:40:36:
> so injecting a dependency with TEA helping with it

should read: with*out* TEA

mr_calvin added on 2020-05-19 10:38:56:
Thanks for taking the time to discuss with me. :)

> Since libtommath is now an external library, just as zlib.

I see that, but an important difference between libtommath and zlib is, though, Tcl's public API does not expose native data types of zlib (z_stream) directly, but proper wrappers (Tcl_ZlibStream). This is different for libtommath (mp_int). There is no Tcl_MpInt, unfortunately.

> why would you still want to build a stripped-down version into Tcl????

This does not apply to NSF, twapi and others (as proper Tcl extensions), but I might want to use Tcl as my C utility library of choice (Tcl is heavily underestimated in this role), maybe, and why then entertain two dependencies if I could just work with one = Tcl with internal libtommath?

> I could imagine TEA providing a TEA_TOMMATH macro

That would certainly help, and was actually what I meant by my -I rumbling in the earlier post.

Nevertheless, I agree that directly depending on tommath.h is likely to best step for NSF, twapi etc. but it does not feel right re Tcl's public API exposing libtommath data types directly (so injecting a dependency with TEA helping with it).

I hope I could make my point clearer?

Thanks for your continued support!

jan.nijtmans added on 2020-05-19 09:31:33: (text/x-fossil-wiki)
> Why doesn't Tcl provide this compat/tommath.h   ...

Since libtommath is now an external library, just as zlib.  The provided <tommath.h> is - starting with 8.7 - maintained externally. As soon as libtommath 1.3 is released, Tcl can be compiled with it instead of the built-in.  My hope is that - in the coming years - libtommath 1.2 will be provided by more (Linux) distributions (MacOS already has it in homebrew now), so installation of libtommath will become less of a burden.  I - actually - don't see the need for the --without-system-libtommath option: If a suitable external libtommath is available, why would you still want to build a stripped-down version into Tcl????

So, yes, it's a burden. I don't think Tcl should take care of installing <tommath.h>, since whenever a newer libtommath version is released the newer version has preference. libtommath's maintenance is extenally now, Tcl should follow it. Tcl providing a stripped-down internal version is just a service. The <tommath.h> included by Tcl is just a fallback in case someone doesn't want to get it somewhere else.

I could imagine TEA providing a TEA_TOMMATH macro, which determines the include/linker flags depending on how Tcl is built. But there are not so many extensions with this problem, NSF is about the only one I know of. Well,  searching for it, I found "tclral" and "twapi" too, so that makes 3. Depending on an external libtommath would make things a lot easier.

I'll think about what can be done to improve the situation.

mr_calvin added on 2020-05-18 21:44:26:
> +   typedef size_t mp_int[4];
> Tcl_DecrRefCount(Tcl_NewBignumObj(&bignumValue));

Thanks for this suggestion, I was entertaining a similar thought. Still, I am wondering about TIP 538 and the public Tcl API (please correct me if I am mistaken):

Tcl_GetBignumFromObj & friends build on mp_int,  functions offered by Tcl as part of its public API but starting with TIP 538, there are no mp_* definitions provided by Tcl itself anymore. An extension or a program integrating with Tcl need to provide a compat definition as a drop-in replacement (as you describe). Is this really necessary and as intended?

> Well, Tcl doesn't install <tommath.h> because it could lead to a conflict with the official libtommath version.

The alternative being that every tclTomMath.h integrator is on its own regarding tommath.h?

If my program or extension was built against Tcl with embedded tommath, it should provide the necessary definitions, doesn't it? Why doesn't Tcl provide this compat/tommath.h (taken from the bundled libtommath sources) if a systemwide libtommath is not available or overriden by --without-system-libtommath (the latter is described in the TIP, at least):

So, a TEA-based program or extension would be set up in a way so that, when built against a Tcl with TCL_WITH_EXTERNAL_TOMMATH undefined, it receives compiler flags incl. -I</path/to/tcl/includes/compat/> so it picks up the tommath.h matching the built-in tommath sources. If TCL_WITH_EXTERNAL_TOMMATH was defined, TEA would not deliver this extra -I  flag, so the systemwide tommath.h would get picked up.

I don't see how this would introduce conflicts (unless a developer messes with the flags on her own, not using TEA, resorting to Tcl's compat/tommath.h on false grounds).

What do you think?

jan.nijtmans added on 2020-05-18 19:50:48: (text/x-fossil-wiki)
Another (little bit hacky) idea: The only libtommath function used is:
     mp_clear()
This is the same as:
    Tcl_DecrRefCount(Tcl_NewBignumObj(&bignumValue));
(Just transfer the bignum to a temporary Tcl_Obj, and clear that one....)

So, this way, you event don't need <tclTomMath.h> any more. The only thing you have to provide for Tcl 8.7 is the mp_int definition, but just some big enough storage is enough.

Here is the patch for nsf:
<pre>
diff --git a/generic/nsf.c b/generic/nsf.c
index 99a2f55c..43242487 100644
--- a/generic/nsf.c
+++ b/generic/nsf.c
@@ -16652,7 +16652,6 @@ Nsf_ConvertToInt32(Tcl_Interp *interp, Tcl_Obj *objPtr,  const Nsf_Param *pPtr,
  *----------------------------------------------------------------------
  */
 
-#include <tclTomMath.h>
 int Nsf_ConvertToInteger(Tcl_Interp *interp, Tcl_Obj *objPtr,  const Nsf_Param *pPtr,
                      ClientData *clientData, Tcl_Obj **outObjPtr)
   nonnull(1) nonnull(2) nonnull(3) nonnull(4) nonnull(5);
@@ -16682,6 +16681,9 @@ Nsf_ConvertToInteger(Tcl_Interp *interp, Tcl_Obj *objPtr,  const Nsf_Param *pPtr
      */
     result = TCL_ERROR;
   } else {
+#if TCL_MAJOR_VERSION > 8 || TCL_MINOR_VERSION > 6
+   typedef size_t mp_int[4];
+#endif
     mp_int bignumValue;
 
     /*
@@ -16696,7 +16698,7 @@ Nsf_ConvertToInteger(Tcl_Interp *interp, Tcl_Obj *objPtr,  const Nsf_Param *pPtr
           }*/
 
     if ((result = Tcl_GetBignumFromObj(interp, objPtr, &bignumValue)) == TCL_OK) {
-      mp_clear(&bignumValue);
+      Tcl_DecrRefCount(Tcl_NewBignumObj(&bignumValue));
     }
   }
 
@@ -35458,9 +35460,6 @@ Nsf_Init(
     if (Tcl_InitStubs(interp, "8.5", 0) == NULL) {
       return TCL_ERROR;
     }
-    if (Tcl_TomMath_InitStubs(interp, "8.5") == NULL) {
-      return TCL_ERROR;
-    }
     stubsInitialized = 1;
   }
 #endif
</pre>

mr_calvin added on 2020-05-18 16:53:58:
Hi Jan!

Thanks for the informative reply, first time I fully realize why there are so many "void *"s in Tcl's public API :)

Sounds like a lot fuzz managing this dependency, just for for checking whether Tcl_BigBignumFromObj suceeds or fails (NSF is not interested in its actual mp_int value). I'd wish there would be Tcl_*BignumFromObj variant for that, or bigValue accepting null (so that mp_* memory management would be taken from the shoulders of the caller).

jan.nijtmans added on 2020-05-18 13:42:09: (text/x-fossil-wiki)
Thanks, Stefan, for this report.

The first problem with TIP #538 is that <tommath.h> is binary incompatible in Tcl 8.7 compared to Tcl 8.6. This is mainly due to Tcl's bad decision to make the mp_digit type 32-bit always: On 64bit platforms this is not optimal, and it's different from how libtommath's build scripts work. So, TIP #538 fixes this: The libtommath included by Tcl is now binary compatible with an externally-built libtommath. The recommended way for extensions would be to do the same thing as Tcl does. So, if Tcl is compiled against an external libtommath, the extension should be linked to that external libtommath as well. There is a symbol TCL_WITH_EXTERNAL_TOMMATH you can use to detect the difference. On many Linux systems (e.g. Ubuntu 20.4 LTS, Focal Fossa) libtommath can be used by "sudo apt-get install libtommath-dev". That shouldn't be too difficult. I expect that Tcl 8.7 - once included in Ubunto - will use this libtommath in stead of it's own version.

So, nsf.c should do:
<pre>
ifdef TCL_WITH_EXTERNAL_TOMMATH
# include <tommath.h>
#else
# include <tclTomMath.h>
#endif
</pre>
and later in the code:
<pre>
#ifndef TCL_WITH_EXTERNAL_TOMMATH
  if (Tcl_TomMath_InitStubs(interp, "8.5") == NULL) {
    return TCL_ERROR;
  }
#endif
</pre>

When linking the library, use "-ltommath" as additional link option.

Of course, NSF could decide to keep it as-is, no problem. The only limitation is that using <tclTommath.h>, only the functions included by Tcl are usable. Using <tommath.h> the full list of libtommath functions can be used. So, in the long run, using an external libtommath is better always.

That doesn't solve the problem described here. Well, Tcl doesn't install <tommath.h> because it could lead to a conflict with the official libtommath version. So, if you want to use the Tcl-provided libtommath, one way to do that is add a "compat" directory to NSF and put Tcl's <tommath.h> there. Just add "-I ../compat" to the compiler options and everything should work.

It would have been nice when Tcl could provide the "mp_int" type from tcl.h. But that would cause a conflict between tcl.h and the external tommath.h. A tiny patch to tommath.h would fix this (See [https://github.com/libtom/libtommath/pull/473|See libtommath's PR 473]), but this PR was rejected with the remark: "No, such patches, which are only useful for Tcl won't happen.".

The tommath.h provided in NSF's "/compat" directory could be a stripped-down version, if you like. Only providing "mp_int" and "mp_digit" is already enough (I didn't test that).

Hope this helps. I'll keep this ticket open, so you can ask more questions if you like. That could be useful for other extensions too.