[Tcl-bugs] [ tcl-Patches-1055668 ] removal of exported internals from tclInt.h (EXTERN macro)

[SourceForge.net <no...@so...>]

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-31 10:19

Message:
Logged In: YES 
user_id=7549

FWIW, Tk_MainEx is in the Stubs table for Tk:  Generic slot 212

----------------------------------------------------------------------

Comment By: David Gravereaux (davygrvy)
Date: 2004-10-31 01:13

Message:
Logged In: YES 
user_id=7549

tktest fixed.  Added those 7 to tkInt.decls.  It took a
little type trickery with some forward declarations in
tclInt.h, but looks safe.  Results are now with patch:

D:\tcl_workspace\tk_head_intern\win>dumpbin /exports
release\tk85.dll | findstr
/C:"number of"
         558 number of functions
         558 number of names

----------------------------------------------------------------------

Comment By: David Gravereaux (davygrvy)
Date: 2004-10-31 00:03

Message:
Logged In: YES 
user_id=7549

Found this error:
        link -nologo -machine:IX86 -release -opt:ref
-opt:icf,3 -opt:nowin98 -su
bsystem:windows -out:".\Release\tktest.exe" kernel32.lib
user32.lib gdi32.lib ".
.\..\tcl_head_intern\win\Release\tcl85.lib"
.\Release\tk_Dynamic\tkTest.obj .\Re
lease\tk_Dynamic\tkSquare.obj
.\Release\tk_Dynamic\testMain.obj .\Release\tk_Dyn
amic\tkWinTest.obj ".\Release\tk85.lib"
tkTest.obj : error LNK2001: unresolved external symbol
_TkpTestembedCmd
tkTest.obj : error LNK2001: unresolved external symbol
_TkTextPrintIndex
tkTest.obj : error LNK2001: unresolved external symbol
_TkTextSetMark
tkTest.obj : error LNK2001: unresolved external symbol
_TkTextIndexBackBytes
tkTest.obj : error LNK2001: unresolved external symbol
_TkTextIndexForwBytes
tkTest.obj : error LNK2001: unresolved external symbol
_TkTextGetIndex
tkTest.obj : error LNK2001: unresolved external symbol
_TkTextMakeByteIndex
.\Release\tktest.exe : fatal error LNK1120: 7 unresolved
externals

Same as Tcl, the test suite is accessing some internal
function not in the private Stubs table.  tkpatch.zip is no
good, will fix and post another.



----------------------------------------------------------------------

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