From: SourceForge.net <no...@so...> - 2004-10-31 06:25:50
|
Patches item #1055668, was opened at 2004-10-27 14:46 Message generated for change (Comment added) made by davygrvy You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=310894&aid=1055668&group_id=10894 Category: 50. Portability Support Group: None Status: Open Resolution: None Priority: 5 Submitted By: David Gravereaux (davygrvy) Assigned to: David Gravereaux (davygrvy) Summary: removal of exported internals from tclInt.h (EXTERN macro) Initial Comment: This patch report is to contain discussion of the current scope problem with regards to internal functions that are being exported by-way of improper use of the EXTERN macro within tclInt.h. There will be a patch file shortly. ---------------------------------------------------------------------- >Comment By: David Gravereaux (davygrvy) Date: 2004-10-30 23:25 Message: Logged In: YES user_id=7549 Just attached the matching patch for Tk. With patch: D:\tcl_workspace\tk_head_intern\win>dumpbin /exports release\tk85.dll | findstr /C:"number of" 551 number of functions 551 number of names Without patch: D:\tcl_workspace\tk_head_stock\win>dumpbin /exports release\tk85.dll | findstr / C:"number of" 824 number of functions 824 number of names ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-30 20:16 Message: Logged In: YES user_id=7549 With patch: D:\tcl_workspace\tcl_head_mod\win>dumpbin /exports release\tcl85.dll | findstr /C:"number of" 704 number of functions 704 number of names Without patch: D:\tcl_workspace\tcl_head_stock\win>dumpbin /exports release\tcl85.dll | findstr /C:"number of" 974 number of functions 974 number of names ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-30 19:39 Message: Logged In: YES user_id=7549 Ok, Tcl_Main returned to as is. New patch attached. This now uses TCL_INTERN() to declare internal functions not destined to be exported (nor imported by a client of the API). I decided to have the macro take the return type as an argument because some special attributes such as calling convention are placed after the return type. I'mjust trying to think into the future by doing this. I also decided to change the use of EXTERN within the tcl*Decl.h files to be TCL_EXTERN() and take the return type as an argument as well. To check the viability of this modification, I appended __cdecl to the #defines like so: #define TCL_EXTERN(RTYPE) TCL_EXT_EXTERN_C TCL_STORAGE_CLASS RTYPE __cdecl #define TCL_INTERN(RTYPE) TCL_INT_EXTERN_C RTYPE __cdecl The patch does not contain this, as I don't feel it isn't needed, but was only used as a check should one day, additional attributes might be placed there (see tip#60 prose; ignore the old patch it contains). Also used Daniel Steffen's idea to replace 'extern' with '__private_extern__' when declaring shared, but not exported functions. It is untested and will need a configure check to set HAVE_PRIVATE_EXTERN. I decided to place the #define of EXTERN within a #ifndef TCL_NO_DEPRECATED/#endif block. I can take it out of that block if it is thought not to be proper. Summary: It looks good and the three classes of public, private, and internal look clear to me now. I hope this helps others working on the core of the distinct difference between a private function in the Stubs table and an internal function not exported. That distinction was never clear before. ---------------------------------------------------------------------- Comment By: Kevin B KENNY (kennykb) Date: 2004-10-30 11:23 Message: Logged In: YES user_id=99768 Can we please agree to constrain scope - on this particular patch - to the externals that are exported accidentally? My opinion is that incompatible change to the export mechanism for Tcl_Main requires a TIP. The other two hundred symbols are much less controversial - please let us not throw that particular baby away with the bathwater. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 13:38 Message: Logged In: YES user_id=7549 dgp> Are you raising 540378 now? I figured-out how to backload it. I'm fine with the change. You did have to invent a new path of development and how to move privates to public scope. je> How is the second version an improvement over the first? All publics are set by Tcl_InitStubs and don't contain an exception. An exception found not to useful for those using Stubs with applications which was beyond the foresight of the author of Stubs. ---------------------------------------------------------------------- Comment By: Joe English (jenglish) Date: 2004-10-28 13:26 Message: Logged In: YES user_id=68433 > Don't pull the "it's been that way forever" arguement, it lacks merit. OK, then ignore the version-independance issue. And the "more complicated" and "less perspicuous" parts too. How is the second version an improvement over the first? ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2004-10-28 13:19 Message: Logged In: YES user_id=80530 Are you raising 540378 now? ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 12:51 Message: Logged In: YES user_id=7549 PS. if you build shells for both implicit linking and -DUSE_TCL_STUBS you will have a hard time using additional Stubs tables from other extensions. Try it.. It doesn't work. Something about Tcl_PkgRequireEx in those Stub static libs don't have the right linkage. I forgot the exact conditions; it's been 5 years since I found it. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 12:45 Message: Logged In: YES user_id=7549 Hell, I even re#define the whole namespace APIs back to the private tables, not to fix Tcl, but to allow version indepedence though additional work. Placing the namespace API into public scope was a very needed change, but I don't consider the namespace APIs to start when they got moved to public scope. Same for Tcl_Main. Because a vector doesn't exist for it in the Stubs table, doesn't mean I can't get at it (thankfully), but my desire for it alone is grounds for being in the Stubs table whether the hacks still need to be in my code or not to support versions prior to it getting placed into the Stubs table. see http://groups.google.com/groups?selm=37A8134C.2847%40mailserver.hursley.ibm.com for Paul's conclusion ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 12:26 Message: Logged In: YES user_id=7549 Don't pull the "it's been that way forever" arguement, it lacks merit. You fail to see the rest of the Tcl API by realizing the one call to Tcl_InitStubs sets everything (minus Tcl_Main). In your manner, which is identical to crazyALT2.c, the addition step is needed to retreive the address of Tcl_Main. Though you didn't say whether both implicit *and* Stubs linkage are in use by your example. It would be hellish to have to GetProcAddress for all Tcl functions should -DUSE_TCL_STUBS not be used. Seeing that one would need that work anyways to support the whole 8.1+ range given that Tcl_Main might appear in the Stubs is no argument that it should stay that way. ---------------------------------------------------------------------- Comment By: Joe English (jenglish) Date: 2004-10-28 11:58 Message: Logged In: YES user_id=68433 Currently, to build a version-independent shell that dynamically loads Tcl, you can use: + hTcl = LoadLibrary(...path to tcl8X.dll...) + tclMainPtr = GetProcAddress(hTcl, "Tcl_Main") + (*tclMainPtr)(argv,argc, yourAppInitProc); In the scheme you propose, you would instead: + hTcl = LoadLibrary( .... ) + tclCreateInterpPtr = GetProcAddress(hTcl, "Tcl_CreateInterp"); + tempInterp = (*tclCreateInterpPtr)(...); + Tcl_InitStubs(tempInterp); + Tcl_DeleteInterp(tempInterp) + Call Tcl_Main() through the stubs table. This is more complicated, less perspicuous, and isn't even version-independant since it would only work for versions of the core where Tcl_Main is in the stubs table. How is the second one an improvement? I just don't see it. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 10:57 Message: Logged In: YES user_id=7549 To reference Paul Duffin himself on the issue of Tcl_Main, see http://groups.google.com/groups?selm=pgpmoose.199901141048.27495%40non.non.net for this: " The stub interfaces of Tcl and Tk provide access to almost all of the external and internal functions both generic and platform dependent. The exceptions to these rules are ones which it makes no sense to make available through the stub interface. These include all Tcl command functions plus the following functions: Tcl_Main Tcl_AppInit Tk_Main " Tcl_Main is not there not because it is bad, but from the point-of-view of only using Stubs linkage for extensions it makes no sense. But Stubs linkage can be used for applications, too. If, on the other hand, people have gotten use to this "restriction" in such a way as to repair it would cause bad side-effects for those that expect Tcl_Main to only have implicit linkage irregardless of -DUSE_TCL_STUBS, then it becomes a sad day when problems are unable to be repaired. http://groups.google.com/groups?selm=37A8134C.2847%40mailserver.hursley.ibm.com ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 10:21 Message: Logged In: YES user_id=7549 >Strictly speaking I don't *have* to use >-DUSE_TCL_STUBS when compiling the main .c file, but I do > anyway just 'cause I always use -DUSE_TCL_STUBS. That doesn't buy you the version independance offered by Stubs. Though on unix, you can probably get around it, but not on win as the version number is tied to the name of the dll at link-time due to Tcl_Main requiring the import library. >Converse question: If you're happy using LoadLibrary() / > GetProcAddress() to call Tcl_CreateInterp(), why isn't the > same technique acceptable for Tcl_Main()? If it was already in the Stubs table, I wouldn't need to do it. The original reason that Paul left it out was because he didn't consider non-extensions would use Stubs. And Tcl_Main has no place with extensions. > I really don't see any advantages to the technique in > crazy*.c, and plenty of disadvantages. Fire away, I'll counter each disadvantage with a balanced advantage. The main advantage is version independence as I can build to 8.1 and load into any 8.x Tcl. A good second is that I have control over what core to load at run-time rather than link-time. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 10:06 Message: Logged In: YES user_id=7549 Attached crazyALT2.c as the example for how to currently access Tcl_Main when using a Stubs-enabled executable. ---------------------------------------------------------------------- Comment By: Joe English (jenglish) Date: 2004-10-28 10:01 Message: Logged In: YES user_id=68433 > jenglish: show me an example in code where someone builds a > shell and has defined USE_TCL_STUBS, but expects Tcl_Main to > be linked implicitly. I've always built shells this way (well, since 8.1 anyway); I thought it was pretty much conventional (i.e., main() just calls Tcl_Main and passes in the address of the local AppInitProc). Strictly speaking I don't *have* to use -DUSE_TCL_STUBS when compiling the main .c file, but I do anyway just 'cause I always use -DUSE_TCL_STUBS. Converse question: If you're happy using LoadLibrary() / GetProcAddress() to call Tcl_CreateInterp(), why isn't the same technique acceptable for Tcl_Main()? I really don't see any advantages to the technique in crazy*.c, and plenty of disadvantages. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 09:31 Message: Logged In: YES user_id=7549 jenglish: build crazyALT1.c against 8.4 and you'll see at link-time you need to specify tcl84.lib (the import library) along with tclstub84.lib. By Tcl_Main being the "bad apple" in the basket, and requires the import library, the version indepedence that Stubs allows us was destroyed. Though users of this style probably got smart and probably switched to a manual LoadLibrary and got the address to Tcl_Main with GetProcAddress when looking for Tcl_CreateInterp, too. I'd do that given that the current "no access" is ilfounded when Stubs was only thought to used for extension only. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 09:02 Message: Logged In: YES user_id=7549 jenglish: show me an example in code where someone builds a shell and has defined USE_TCL_STUBS, but expects Tcl_Main to be linked implicitly. IOW, Tcl_Main is through implicit linkage and their Tcl_Init is by way of the Stubs table. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 08:56 Message: Logged In: YES user_id=7549 dkf> If Tcl_Main() was stubbed, apps would be unable to call it dkf> without first initializing the stub table reference. Untrue! Only if they were using the Stub interface. implicit linking is as normal by not specifying USE_TCL_STUBS. Just like all other exported functions. The standard shells are not build with -DUSE_TCL_STUBS. dkf> This breaks existing users of the API, for whom the call to dkf> it is probably the only thing they have in their main() at the dkf> moment. Again, untrue! The only difference is that Tcl_Main is defined to (tclStubsPtr->tclMain) for when USE_TCL_STUBS is defined. It never never had an entry in the table before. Without USE_TCL_STUBS, it is as it was regarding visibility. ---------------------------------------------------------------------- Comment By: Joe English (jenglish) Date: 2004-10-28 08:50 Message: Logged In: YES user_id=68433 > Actually, it isn't an interface change at all. The scope is > unchanged being as public as it was before. It's a big change at the source level: after #include <tcl.h>, the symbol 'Tcl_Main' would be a macro that dereferences a (unititialized) function pointer, instead of a public function name. This would cause catastrophic failures for nearly all current users of Tcl_Main(). ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 08:44 Message: Logged In: YES user_id=7549 But extension aren't the only thing tcl is used for. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2004-10-28 08:38 Message: Logged In: YES user_id=80530 check the Tcl_Main() documentation. Tcl_Main() is explicitly not available to extensions. It's also not thread-safe, so unsuitable for stub-based access. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 08:35 Message: Logged In: YES user_id=7549 Actually, it isn't an interface change at all. The scope is unchanged being as public as it was before. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2004-10-28 06:26 Message: Logged In: YES user_id=80530 dkf's last comment should be conclusive on the point that adding Tcl_Main() to the public stub table is an interface change, needing a TIP, and should be out of scope in this bug-fixing patch. I'll save my arguments for when that arrives. ;) Thanks for the work cleaning up the internal stuff though! Passing control to another pair of eyeballs. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-10-28 06:09 Message: Logged In: YES user_id=79902 If Tcl_Main() was stubbed, apps would be unable to call it without first initializing the stub table reference. This breaks existing users of the API, for whom the call to it is probably the only thing they have in their main() at the moment. Your examples are all very well, but they run smack into the fact that current users of Tcl_Main() would be significantly negatively impacted for no gain that they'd see. It's very much an exception! Assigning to someone else to argue with you that stubbing Tcl_Main is a bad idea... ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 04:48 Message: Logged In: YES user_id=7549 here's another done GUI flavor. No main window is present as I don't feel like merging event loops by calling Tcl_DoOneEvent(TCL_DONT_WAIT) from a WM_IDLE and WM_TIMER and the nastiness it gets into. Imagine for a moment that Tcl is being run in a thread with async console reading seperate from the GUI's message pump and is a full featured GUI in MFC or wxWindows. AllocConsole() from a GUI app is pretty neet, eh? ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 03:46 Message: Logged In: YES user_id=7549 dkf: here's some fun.. Have a look at crazy.c I just attached. If one starts that "shell" with the path to tcl85.dll built by this patch, Tcl_Main has an entry in the table and Stubs works. I could have attained the address to Tcl_Main with GetProcAddress, but I'm trying to make a point that Stubs should be for eveyone and no publics should escape. Even seen anyone use Stubs like that? ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 03:09 Message: Logged In: YES user_id=7549 Lets be clear on scope again. Here are the levels worded a bit differently: 1) Public functions in the Stubs table. - Full privilege, the exported API. - ex. Tcl_CreateInterp 2) Private functions in the Stubs table. - available to the user of the API and exported, but no guarantee across versions as they could change. - ex. TclDeleteCompiledLocalVars 3) Internal functions and variables shared across the sources, but not exported. - no access from the outside. - ex. TclFinalizeCompExecEnv 4) File scope variables and functions not intended to be accessed outside the source file they are declared in. - no access from the outside. - ex. BuildCommandLine from win/tclWinPipe.c and struct ThreadSpecificData found in almost every source file. Lets call these problem functions the 'internal' (#3) ones and the exported ones from tclInt.decls the 'private' #2 ones. All functions that fit class#3 need to have their accidental exporting removed. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 02:05 Message: Logged In: YES user_id=7549 Actually, no we haven't discused this before. Placing Tcl_Main in the Stub also exports from the dll, so it is the same. It just so happens to have a vector in the table. Whether an extension is to use it or not is neglegable. True, it doesn't make sense from the point-of-view of an extension, but people also use Stubs with applications. Granted, without having interp to request it's table, it isn't obvious. People who LoadLibrary"tclXX.dll), then GetProcAddress(hTcl, "Tcl_CreateInterp") then Tcl_InitStubs can get the table that way. I can show you examples in code if you prefer. Also search google for c.l.t. posts by Paul Duffin around the `99 timeframe where someone showed him the usefulness of Tcl_Main being in the table. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 01:48 Message: Logged In: YES user_id=7549 new patch with stuff I forgot and Tcl_Main moved to the public Stubs table. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-10-28 01:30 Message: Logged In: YES user_id=79902 Tcl_Main must not go in the stubs table, but it must be a public API function of the core. It's only ever intended for code that is linked against tcl.dll (add your version number of choice) directly. (I believe we've been through the loop on that item many times before.) ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-28 00:25 Message: Logged In: YES user_id=7549 Notes to self: 1) tcl.h(2330) Tcl_Main is not in the Stubs table, but is exported. What to do?.. Probably drop it into tcl.decls like it should have always been. 2) unix\tclXtNotify.c(97) TclSetAppContext is being exported from there.. hmm.. No clue what should be done. 3) unix\tclLoadShl.c(22) yet another reason to rename EXTERN to TCL_EXTERN, but this time do it the right way. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-27 18:32 Message: Logged In: YES user_id=7549 whoop! My mistake. exports count is 706. The savings is 245. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-27 18:12 Message: Logged In: YES user_id=7549 exports reduced to 739 from 951 saving us from exporting 212 functions that weren't "blessed" by being in the Stubs table. ---------------------------------------------------------------------- Comment By: David Gravereaux (davygrvy) Date: 2004-10-27 18:07 Message: Logged In: YES user_id=7549 initial patch uploaded. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=310894&aid=1055668&group_id=10894 |