Tcl Source Code

Here are two patches, which together simply winMain.c, eliminating
most of the win32-specific code from it (everything except the different
entry point and the setargv workaround). The need for this win32-specific
code was that wish wants to install a panic proc as soon as
possible, as win32 gui applications don't have stdout/stderr. This way,
all embedders need to do the same for gui-based application. My
proposal is to move this to Tk_Main(). On UNIX there is no change.

The patch for Tcl, modifies Tcl_FindExecutable() (the first function that
Tk_Main calls) such that calling Tcl_FindExecutable(NULL) installs
a panicproc equivalent to the current WishPanic(). This NULL must
be seen as a magic value, any other value would be OK too. On
Win32, the argment is just a dummy, so this is 100% upwards
compatible.

The patch for Tk, modifies the Tk_Main macro on win32 to call
Tcl_FindExecutable(NULL) in stead of Tcl_FindExecutable(argv0).
The only potential incompatibility occurs when embedders call
Tcl_SetPanicProc before calling Tk_Main: Then the user-
supplied panicproc will be replaced by the standard panic proc.
workaround: embedders can call Tcl_SetPanicProc just before
Tcl_Init(), that will work in earlier Tcl versions as well.

Another minor change this patch makes, is changing
the string "Application initialization failed", to
"application-specific initialization failed", so the
message is equal in tclsh and in wish. But maybe
another message would be better.

Please, evaluate this, if it is suitable for Tcl8.6.

Regards,
        Jan Nijtmans

Restored the possibility to define a panicproc as
low memory handler See: [#1446864]

allow_comments - 1

doc.patch committed to trunk

File Added - 418967: doc.patch

File Deleted - 418965:

Here is the updated documentation (doc.patch). Please evaluate
whether this is an accurate and understandable description.

File Added - 418965: doc.patch

>First, generic/tclPanic.c has now grown
>a significant amount of #ifdef _WIN32 .
>I'd like things much better if we could keep the
>generic code truly generic.

Noted. However the biggest _WIN32
section is the end of Tcl_PanicVA(), and
this part comes from the fact that M$
didn't implement abort() the way it
is supposed to work: provide an entry
point to the debugger. Given the
fact that there is only one abort()
call left in Tcl/Tk, this is the most
logical place to do it. Implementing
a TclAbort internal function would
not make the code more readable,
so I prefer keeping it as is.

> Second, the interface of Tcl_FindExecutable()
> has been changed by the patch
In fact T_FE(NULL) is only intended to be
used by the Tk_Main() macro, not by
anyone else. The fact that the T_FE
argument has always been a dummy
argument on Win32 has never been
documented either. But I agree that
it is better to document this. How
about something like:

On Win32, the argv0 argument has
no effect, except for the default
panicproc installed. If either a
debugger is running or Tcl_SetPanicProc
is called, whatever value of argv0 has
no effect. Otherwise a NULL value
installs a GUI dialog as default
panic proc while any non-null value
results in the panic message to be
written to stderr before entering the
debugger.

That's the short summary of what
this patch is actually doing.

Finally had a chance to look at this series of
patches.  I don't really have the qualifications to
comment on what it does and why.  If the code has
reached a state where both nijtmans and ferrieux are
satisfied, I'll accept that as good enough.

That said, I have a few concerns.

First, generic/tclPanic.c has now grown
a significant amount of #ifdef _WIN32 .
I'd like things much better if we could keep the
generic code truly generic.

Second, the interface of Tcl_FindExecutable() 
has been changed by the patch.  T_FE() is meant 
for embedders, and typically they should pass the argv0
from their program's main().  However, some embedding
scenarios don't easily have access to that, and it's been
an option to pass NULL if you don't have a better value 
available to pass in.  Now that option has additional
function attached to it, at least on one platform.  We
should at least note the incompatibility.

I want to look at this again.  Can't follow through
immediately.  Raising prio so I don't forget.

File Added - 396279: tkpDisplayWarning.patch

Another addition (or am I getting too enthousiastic....)
TkpDisplayWarning is used to display warnings as
well (but non-critical). This function could be adapted
to talk to the debugger as well, that would prevent
an (ugly) MessageBox to come up.

Here is a patch doing that (tkpDisplayWarning.patch)

Compare that with Tcl 8.5:
======================================================
$ gdb tcltest.exe
GNU gdb (GDB) 7.2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from tcltest.exe...(no debugging symbols found)...done.
(gdb) run
Starting program: tcltest.exe
[New Thread 2388.0xd30]
The Panic Message

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.

Program exited with code 03.
(gdb)

(1)  In tclsh, you will never see a MessageBox, that's only for
wish when there is no debugger. The panic string will
be printed to stderr.
(2) See below. It looks like gdb implements Microsoft's
API as well. That's the point: Tcl/Tk compiled with mingw
should be 100% binary compatible with Tcl/Tk compiled
with MSVC.

================================
$ gdb tcltest.exe
GNU gdb (GDB) 7.2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from tcltest.exe...(no debugging symbols found)...done.
(gdb) run
Starting program: tcltest.exe
[New Thread 5864.0x1310]
warning: The Panic Message

Program received signal SIGILL, Illegal instruction.
0x6d5a9d91 in Tcl_PanicVA ()
   from C:\\workspace\tcl8.6\win\tcl86.dll
(gdb)

OK for the unix case, things are correct (abort() is back ;-).
For the Win32 case, I'm not sure. I have two questions:

(1)  if I debug tclsh under gdb inside emacs in mingw, will I have or not a MessageBox that pops in my face and that I need to dismiss before the debugger gets the breakpoint ?

(2) What sill I see in the gdb: Illegal Instruction ? Aborted ? The string passed to Panic ?

Done as asked, with two additional improvements:
- If a debugger is there, use DebugOutputStringW to
  communicate the panic message to the debugger,
  otherwise use the MessageBox as before.
  (Thanks, Alex, for the idea to check for presence
  of the debugger, then this is a logical follow-up!)
- Built in a protection in Tcl_SetPanicProc(), to
  resolve the potential incompatibility mentioned
  in the description of this issue: Tcl_SetPanicProc
  wil never override an externally set proc with an
  internal one.

I think everything is resolved now as discussed.

Thanks!

No, that's still no good. In the unix case + memdebug, tclsh, we now have *both* a __builtin_trap then an abort(), so we're exiting with "Illegal instruction" as you said.

So please:

  - on unix, just abort(), always, regardless of memdebug

  - on windows, always try DebugBreak(), regardless of memdebug. If no debugger is here, then you may pop some GUI (because the developer is not here) if you really want, and then ExitProcess().

Here are two new patches, which address the things discussed here:

- Currenty (Tcl 8.5)  a panic on Windows results in the message:
    This application has requested the Runtime to terminate it in an unusual way.
    Please contract the application's support team for more information.
  Changing abort() to ExitProcess(1) brings the behavior on Windows more
  in line with the behavior on UNUX, which simply prints "Aborted"

- Wish on Windows tries to activate a breakpoint in the
  debugger (either VS or GDB) when panicing. This is not
  logical for an end-user machine (as ferrieux already noted).
  Suggestion: Only try to trigger the debugger in debug
  mode (or does that sound too trivial....). Documentation
  should be modified on that.

The patch for Tk does nothing more than use the panicproc
dialog build in Tcl, which does exactly the same as the one
previously built in winMain.c. This is no functional change.

Opinions?

File Added - 396202: 3124554_tk.patch

File Added - 396201: 3124554_tcl.patch

File Deleted - 394928:

> this is no change from how it was before

Wrong: moving from Tk to Tcl is not transparent for people using tclsh !!! 
You are breaking the debug path for tclsh.exe.

> Users should never see a panic ;-) but when it happens,
> better display a message just before the eventual crash.

Two situations for an unadorned exception:
 - either there's a debugger installed (developer case), and it breaks into it
 - either it's an end-user machine, and the OS displays a MessageBox itself, on another heap (not the offending process' one, which is not trustable anyway).

So (1) there's no need to add GUI at crash time, and (2)  you didn't address my remarks on the unwanted effects of doing so.

My answer: This patch does in tclWinFile.c what previously was done in
tk: winMain.c.  So, you remarks are valid, but this is no change from
how it was before.

>Idon't see the point of mixing GUI stuff (which is for the user)
> and debugging stuff (which is for the developer).
Users should never see a panic ;-) but when it happens,
better display a message just before the eventual crash.
Wish does not have stderr we can print to.....unfortunately.
Any information that can be extracted from it might be
useful.

Thanks!

Looks like the _tcl patch sets a panicproc that calls GUI functions (like MessageBox). This worries me because:

  (1) If we're panic'ing with a corrupted heap, MessageBox may itself crash, muddying the water.
  (2) Even if it works, MessageBox is called before DebugBreak, possibly also modifying state from the initial panic point.

I don't see the point of mixing GUI stuff (which is for the user) and debugging stuff (which is for the developer).

File Added - 394928: 3124554_tk.patch

File Added - 394927: 3124554_tcl.patch