Tcl package Thread source code

When I compile Tcl 8.6.11 with gcc 10 (didn't try a different version) I get the following warning when linking the tread extension, because the core uses the TclIntStubs typedef whereas the extension uses the struct of the same name.

/[...]/tcl8.6.11/pkgs/thread2.8.6/generic/threadSvListCmd.c:25:34: warning: type of 'tclIntStubsPtr' does not match original declaration [-Wlto-type-mismatch]
   25 | extern const struct TclIntStubs *tclIntStubsPtr;
      |                                  ^
/[...]/tcl8.6.11/generic/tclStubLib.c:23:20: note: 'tclIntStubsPtr' was previously declared here
   23 | const TclIntStubs *tclIntStubsPtr = NULL;
      |                    ^

> I am still curious to learn why it is better in this case to copy parts of data structures instead of ...

Of course, it's a trade-off. But looking at the limitations of LTO, e.g.: It is recommended that you compile all the files participating in the same link with the same options and also specify those options at link time. ... I don't think we want to use LTO crossing the border between Tcl and extensions. This warning is just one example, compiling an extension with different optimizations than Tcl (this is something we don't control) is a dangerous thing to do when LTO is involved.

The good news is that this warning is 100% gone now ;-) which was your condition for closing the ticket.... (which - actually - I agree with)

Thanks!

Yes, with that change the warnings are gone, and so is the link time optimisation for these three files, but I am not sure if that matters in practice.

I am still curious to learn why it is better in this case to copy parts of data structures instead of just including the respective header, so that everything is only declared in exactly one place.

Does this (change in Tcl 8.6) work for you?: https://core.tcl-lang.org/tcl/info/aa4a13c15516da45. Advantage: it works for all extensions using this construct, not only "Thread". (Tk 8.7 uses it too)

I agree that the warning is harmless in this case as long as none of the two declarations gets changed in an incompatible way, but I think our goal is to keep Tcl warning-free, so I am against closing this as long as it hasn't been fixed.

The warning could of course be silenced by adding the -Wno-lto-type-mismatch compiler flag when compiling and/or linking this file, but why do you reject the inclusion of tclInt.h, which would IMO be the cleanest solution?

Even the comment for the copied declaration of TclIntStubs says that it is a hack, and I think the thread extension is so close to the core that using an internal header would be justified.

Thanks for this info. Yes, I can reproduce it now.

I experimented a little bit on trying to disable this warning. Tried various #pragma's, but none of them work. There are - indeed - two different definitions of TclIntStubs which are different: But the first part is the same, the part actually used in this source-file. So this little hack works, I don't see any practial problem. The warning can safely be ignored.

If you have a suggestion (other than using tclInt.h), I'm all ears. Otherwise, I recommend to just close this ticket.

Meanwhie I found that the warning happens only when link-time optimisations are used (-flto=auto -ffat-lto-objects) because only then the linker has type information and can warn about such things. This is the default on openSUSE.

I expected that removing "struct" would help, but actually it doesn't. It seems to be either that there exist two separate declarations for that struct, or that the two structures are actually formally different that triggers the warning.

When I comment out the "typedef struct ..." block in threadSvListCmd.c and include "tclInt.h" instead I don't get the warning regardless of the "struct" keyword being there or not in the following extern declaration for tclIntStubsPtr (which isn't then needed anymore).

But then a new warning pops up wich also looks like a result of copying stuff from headers instead of including them:

In file included from /tmp/tcl8.6.11/pkgs/thread2.8.6/generic/threadSvListCmd.c:27:
/tmp/tcl8.6.11/generic/tclInt.h:1351: warning: "TCL_TSD_INIT" redefined
 1351 | #define TCL_TSD_INIT(keyPtr) \
      | 
In file included from /tmp/tcl8.6.11/pkgs/thread2.8.6/generic/threadSpCmd.h:15,
                 from /tmp/tcl8.6.11/pkgs/thread2.8.6/generic/threadSvCmd.h:19,
                 from /tmp/tcl8.6.11/pkgs/thread2.8.6/generic/threadSvListCmd.c:12:
/tmp/tcl8.6.11/pkgs/thread2.8.6/generic/tclThreadInt.h:125: note: this is the location of the previous definition
  125 | #define TCL_TSD_INIT(keyPtr) \
      | 

Well, I cannot reproduce this on Ubuntu 20.10 (with gcc 10.2). To me it looks like a false alarm, since "tclIntStubs" is typedef'd as "struct tclIntStubs". However, it's easy and harmless to remove the "struct" keyword here.

So, does [7a8d2a95838dcab3|this commit] help?