From: SourceForge.net <no...@so...> - 2009-12-11 10:57:21
|
Bugs item #2902965, was opened at 2009-11-24 07:56 Message generated for change (Comment added) made by nijtmans You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=2902965&group_id=10894 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: 40. Dynamic Loading Group: development: 8.6b1.1 >Status: Pending >Resolution: Wont Fix Priority: 9 Private: No Submitted By: Jan Nijtmans (nijtmans) >Assigned to: Pat Thoyts (patthoyts) Summary: stub related changes cause tclkit built to break Initial Comment: Pat reported me the following. I didn't expect those changes to cause such problems. This issue is meant to work out a solution, for everybody to comment on. Pat Thoyts: I'm not sure this is correct. You now force the dde and registry extensions to use stubs at all times - even when building static. tclkit links static versions of these into the tclkit executable so now I get a link error for unresolved symbol _tclStubsPtr from tclWinDde.obj and tclWinReg.obj when linking the tclkit executable. Obviously I can add the tcl stubs library to the linker command line but really should I need to? Surely in a static binary like this I should only be exposing the stubs interface - not making use of it internally. The original bug you were fixing is irrelevant to the tclkit build as when we construct the vfs we manufacture a static pkgIndex so that package require does the right thing. ie: package ifneeded dde 1.n.n {load {} dde} I could well be wrong, but surely when building a static version of dde/registry I don't need the macro getting undef'd on me. Also, if this breaks tclkit I think we can be sure it will break other builds of embedded tcl elsewhere. ---------------------------------------------------------------------- >Comment By: Jan Nijtmans (nijtmans) Date: 2009-12-11 11:57 Message: Thinking more about it, the change that caused TclKit build to break is that the tcldde and tclreg packages are now compiled with TCL_USE_STUBS, which they were not before. That means that any executable which wants to include those two packages in static form, now needs to link with the stub library. This is exactly the kind of POTENTIAL INCOMPATIBILITY as what was caused by the removal of tclStubsPtr from the tcl library. I tried to fix that by including the stub objects in libtcl.a, but it was (rightfully) reverted by das, because it caused multiply defined symbols on older OSX. The conclusion should be: The tclkit build should be changed to link with the Tcl stub library in this case: It was already needed when linking with tk.lib, now it is needed when linking it with either tcldde.lib or tclreg.lib. Therefore the "Wont Fix" Assigning it to Pat Thoyts for evaluation. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2009-11-30 14:31 Message: Whooooah. Found the real cause of the problem on OSX: In 3 files (tkConsole.c, tkWindow.c and tkMain.c), there are direct calls to Tcl_InitStubs().If USE_TCL_STUBS is not definded, Tcl_InitStubs is a macro which is supposed to be used when Tcl is compiled statically. However, Tk is always compiled with USE_TCL_STUBS, whether Tcl is compiled statically or not. So Tk violates Tcl's rules in this respect!. Hmmm. That's a completely separate problem which has to be fixed first..... ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2009-11-29 23:25 Message: I don;t have an older OSX to try this on, but - yes - breaking a build is bad! I now synced the win/tcl.m4's from Tcl and Tk, hoping (and expecting) everything is OK now. Actually, I don't understand what's the problem including tclSubLib.o in both libtcl.a and libtclstub.a: Linking archives only results in linking the objects which contain symbols which are unresolved yet. (?). But I beleave das on his word that there's a problem on older OSX (yes, I know there are strange machines in this world!) So, I'll leave the tclStubsPtr subject for now until I fully understand what's going on. ---------------------------------------------------------------------- Comment By: Daniel A. Steffen (das) Date: 2009-11-29 10:30 Message: also reverted the unix/tclAppInit.c change after re-reading some of your comments again to try and figure out what this was supposed to be fixing, I think you are confused re tclStubsPtr: > No! For shared libraries nothing changes. > Previously, there were two tclStubPtr variables, > one in the shared library, another in the static > stub library. NO! there is no tclStubsPtr in the tcl shared library anymore, nor in the static tcl library (that was the whole point of the recent work to get stubsPtr out of libtcl), nor in tclsh, the only place to get tclStubsPtr from is the static stubs library. ---------------------------------------------------------------------- Comment By: Daniel A. Steffen (das) Date: 2009-11-29 09:46 Message: I am reverting the unix tcl.m4 change, it breaks static tk builds on older OSX due to multiply defined symbols (cannot link against both libtcl and libtclstub with this change), MAKE_LIB should _not_ include ${STUB_LIB_OBJS} ! (Also note that we've been keeping tcl and tk unix/tcl.m4 in sync, please test any tcl.m4 change made in tcl/unix with tk/unix as well...) If you need to link with the stub objects, explicitly link with the static stub library, that's the only place STUB_LIB_OBJS should be included, they should not be present in the main static lib. We spent a lot of time not that long ago getting this right, I don't understand what this change is supposed to be fixing, but it's undoing all that work for no reason I see explained in this bug as far as unix is concerned (I don't pretend to understand the windows issue, your change may well be valid there). Also, as previously mentioned, tclsh should _not_ link with the stubs library, reverted that change to unix/Makefile.in ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2009-11-27 23:32 Message: Point noted. However, the difficult point is to assure that it works with ALL builds (makefile.vc, win/Makefile, unix/Makefile) before. Kevin rightfully reverted the win/tclAppInit.c change (I was planning to do that myself), but the real problem was a dependancy problem, not the change in tclAppInit.c itself. On Windows, you cannot reference to a variable in a static executable from a dll, so this change was useless! ---------------------------------------------------------------------- Comment By: Pat Thoyts (patthoyts) Date: 2009-11-27 22:39 Message: In my opinion only bug fixes should be going into 8.5. Certainly not anything that might interfere with peoples build systems as this will likely do. The change here seems to be to add the stubs library back into the tcl library when that is being built as a static library. It seems to me that if a build requires the stubs library to be available then it should specify this when linking. So it is that tclkit links with -ltcl8.5 and -ltclstubs. We went though this when dgp removed the stubs objects from the main tcl dll for 8.6a0. We had to fix a load of builds then and I don't much want to do it all again. ---------------------------------------------------------------------- Comment By: Kevin B KENNY (kennykb) Date: 2009-11-27 22:09 Message: Please do not backport. The changes do not fix bugs, they simply (allegedly) improve build cleanliness. They also have made me confront one broken build after another, and I am getting sick and tired of finding that the HEAD fails to build every time I update - and spending hours figuring what you did and restoring it. You have been committing these changes entirely untested. ---------------------------------------------------------------------- Comment By: Andreas Kupries (andreas_kupries) Date: 2009-11-27 22:02 Message: Yes, I object. 8.5.8 is the stable branch, and these changes you are doing currently look anything but stable. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2009-11-26 13:20 Message: I would like to backport the fix (see patch below) to Tcl/Tk 8.5 as well. Just this patch, nothing else (no changes to the tclreg/dde/tcltest build, no changes to the tclStubPtr variable in libtcl.so!). Reason: It would move away the use of tclStubsPtr from libtcl.so, reducing the chance of any ill-behaviour. If some (ill-compiled) extension previously would find tclStubPtr in libtcl.so, now it will find it in tclsh/wish and it is guaranteed to be initialized. Any objections? ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2009-11-26 10:21 Message: Fixed in HEAD (/win and /unix). Tk still to be done. (Remarks/critics/suggestions still welcome!) ---------------------------------------------------------------------- Comment By: Pat Thoyts (patthoyts) Date: 2009-11-25 01:34 Message: > make it possible to build both a static tclreg12.lib > and a tclreg12.dll next to each other When you build a dll you get a link library with it - so creating a tclreg12.dll also creates a tclreg12.lib. The static lib is also named tclreg12.lib although we usually try to add a suffix to help discriminate this from the link library so getting tclreg12s.lib. So creating the two types of library 'next to each other' is something to take care with. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2009-11-24 21:20 Message: >Should I conclude then, that after >we had completed taking tclStubsPtr >out of libtcl, you are putting it back in >again ?!? No! For shared libraries nothing changes. Previously, there were two tclStubPtr variables, one in the shared library, another in the static stub library. That is dangerous, because when static and shared libraries are mixed, it is system dependant what will be the result. There were two different object files which declared the same variable, and that is dangerous. What I want to do is putting the stub library in the static library as well. Still it depends on the situation which objects is included in the final executable: the one from the stub library or the one from the static library. But this time it doesn't matter because both object files are exactly the same! So, the dangerous situation from before is still gone. We can mix static and shared libraries at will, it simply will work always. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2009-11-24 20:41 Message: ??? Haven't examined the patch yet. Just reacting to the last comment. Should I conclude then, that after we had completed taking tclStubsPtr out of libtcl, you are putting it back in again ?!? ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2009-11-24 12:23 Message: OK, here is the fix. Explanation: The reason for the stub changes is to make it possible to build both a static tclreg12.lib and a tclreg12.dll next to each other (and the same for tcldde13.lib/tcldde13.dll). Then we can do the same with the future tcltest86.lib/tcltest86.dll. We would need to compile two tclWinReg.obj files, one compiled without and one compiled with stubs. It's a lot easier to reduce this to only one, it has the advantage that tclreg12.lib can be linked in any tclsh, no matter it is compiled against tcl86.dll or tcl86.lib. Any combination of static and shared libraries will work! The disadvantage is that every executable which includes a single static library will need to link with the stub table as well. Solution: Include the stub library in any static library we produce. Then linking with any static library will automatically include the stub library objects as well. This fixes the tclkit build. proposed patch attached to this issue (/win directory only, in /unix similar changes have to be made) The modification to tclAppInit.c is not necessary, it only makes that tclsh not is forced to link with the stub library, it is completely initialized as well. So, even extensions that are compiled with TCL_USE_STUBS bug forget to call Tcl_InitStubs() will still work. It is meant as an extra safety net. With those changes, it's impossible to produce non-working static executables! ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=2902965&group_id=10894 |