Don't try to redefine the signal() function on Windows

[zturner]

Summary

Windows comes with an extremely limited implementation of signal which only supports a few signal values. Of importance to LLDB is the SIGINT value which invokes a callback when the user presses Ctrl+C. For some reason, we were completely redefining the signal() function to intercept SIGINT and use our own custom processing of Ctrl+C rather than use the builtin processing of Ctrl+C done by signal(). This is frought with peril for a number of reasons:

1. Depending on order of includes, you could get into a situation where both builtin signal() and our own custom signal() are defined, leading to multiply defined symbol errors at link time.
2. It leads to a bunch of compiler warnings, since we end up redefining the same constants (e.g. SIG_DFL etc) because we were going through hoops to avoid #including the standard headers (even though that doesn't work)
3. The standard implementation of signal() actually does exactly what you would expect it to do with regards to Ctrl+C. In fact, by looking at the source code of the CRT, you can see that for SIGINT it actually just calls SetConsoleCtrlHandler, which is exactly what our own custom implementation of signal did!

So, we simply remove all of our custom windows versions of signal() and use the builtin implementation. Code that attempts to call signal() with values that are not supported on Windows are handled by preprocessor defines

[zturner]

Update with context

[cameron314]

Cool! I got pulled into something else at work and didn't have time to investigate the linker error that this led to, sorry. But the patch looks good to me (not that I know LLDB very well).

tools/driver/Driver.cpp

1314 ↗

(On Diff #51082)

Might be slightly more portable to use _WIN32 to detect the OS platform instead of detecting the compiler?

tools/driver/Platform.cpp

93–97 ↗

(On Diff #51082)

It looks like MIDriverMain.cpp was using this. Will it continue to work without the console control handler?

tools/driver/Platform.h

15 ↗

(On Diff #51082)

This comment is no longer relevant.

[amccarth]

I like where this is going. Just a couple concerns.

tools/driver/Platform.h

15 ↗

(On Diff #51090)

delete this comment

40 ↗

(On Diff #51090)

Now that we are including <signal.h>, won't the following defines create duplicate definition warnings for the non-Windows platforms?

tools/lldb-mi/Platform.h

73 ↗

(On Diff #51090)

As in the other file, now that we're including <signal.h>, I think the following definitions may be unnecessary and might cause compile-time warnings for non-Windows systems.

[zturner]

tools/driver/Driver.cpp

1314 ↗

(On Diff #51090)

I think _MSC_VER is the right check, because the builtin signal implementation is something that is part of Visual Studio. If you were using Cygwin I imagine some of those constants might be present (although tbh, I don't ever use Cygwin and I don't know if anyone else does either, so someone would need to confirm).

_MSC_VER is also defined for clang-cl, so this check would catch MSVC and clang-cl

tools/driver/Platform.cpp

93–97 ↗

(On Diff #51090)

Yes, because now MIDriverMain.cpp should end up calling the builtin version of signal(), which also calls SetConsoleCtrlHandler. As far as I can tell there's no actual difference (technically, the builtin version is a strict superset of this version, so if anything it should be better)

[zturner]

tools/lldb-mi/Platform.h

73 ↗

(On Diff #51090)

No, because Windows doesn't define them in the first place. Windows only defines the 5 or 6 values it supports. These are here so as to avoid having to fix up the remaining 10-20 locations and wrap them in pre-processor definitions. signal() will just return an error if called with these values, but it's no different than the old implementation where we simply ignored the values by way of a switch statement that did nothing.

[amccarth]

looks good

[cameron314]

tools/driver/Driver.cpp

1314 ↗

(On Diff #51090)

I was thinking more of mingw-w64, which tries very hard to have a compatible set of headers as those defined in the Windows SDK (and does not define _MSC_VER). But many, many other places in LLDB already check _MSC_VER anyway.

tools/driver/Platform.cpp

93–97 ↗

(On Diff #51090)

Great!

tools/driver/Platform.h

40 ↗

(On Diff #51090)

It's only included on Windows, though -- unless some other TU includes both this header and signal.h?

lldb/trunk/tools/driver/Driver.cpp

#ifdef _MSC_VER
   setvbuf(stdout, NULL, _IONBF, 0);
   setvbuf(stdin , NULL, _IONBF, 0);
 #endif
 
     SBDebugger::Initialize();
     
     SBHostOS::ThreadCreated ("<lldb.driver.main-thread>");
 
+    signal(SIGINT, sigint_handler);
+#ifndef _MSC_VER
     signal (SIGPIPE, SIG_IGN);
     signal (SIGWINCH, sigwinch_handler);
-    signal (SIGINT, sigint_handler);
     signal (SIGTSTP, sigtstp_handler);
     signal (SIGCONT, sigcont_handler);
+#endif
 
     // Create a scope for driver so that the driver object will destroy itself
     // before SBDebugger::Terminate() is called.
     {
         Driver driver;
 
         bool exiting = false;
         SBError error (driver.ParseArgs (argc, argv, stdout, exiting));

lldb/trunk/tools/driver/Platform.h

 //===-- Platform.h ----------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef lldb_Platform_h_
 #define lldb_Platform_h_
 
 #if defined( _WIN32 )
 
-    // this will stop signal.h being included
-    #define _INC_SIGNAL
     #include "lldb/Host/HostGetOpt.h"
     #include <io.h>
 #if defined( _MSC_VER )
     #include <eh.h>
+    #include <signal.h>
 #endif
     #include <inttypes.h>
     #include "lldb/Host/windows/windows.h"
 
     struct winsize
     {
         long ws_col;
     };
 
     typedef unsigned char   cc_t;
     typedef unsigned int    speed_t;
     typedef unsigned int    tcflag_t;
 
     // fcntl.h
     #define O_NOCTTY 0400
 
     // ioctls.h
     #define TIOCGWINSZ 0x5413
 
-
-    // signal handler function pointer type
-    typedef void(*sighandler_t)(int);
-
-    // signal.h
-    #define SIGINT 2
-    // default handler
-    #define SIG_DFL ( (sighandler_t) -1 )
-    // ignored
-    #define SIG_IGN ( (sighandler_t) -2 )
-
     // signal.h
     #define SIGPIPE  13
     #define SIGCONT  18
     #define SIGTSTP  20
     #define SIGWINCH 28
 
     // tcsetattr arguments
     #define TCSANOW 0

 #ifdef _MSC_VER
     struct timeval
     {
         long tv_sec;
         long tv_usec;
     };
     typedef long pid_t;
     #define snprintf _snprintf
-    extern sighandler_t signal( int sig, sighandler_t );
     #define PATH_MAX MAX_PATH
 #endif
 
     #define STDIN_FILENO 0
 
     extern int  ioctl( int d, int request, ... );
     extern int  kill ( pid_t pid, int sig      );
     extern int  tcsetattr( int fd, int optional_actions, const struct termios *termios_p );

lldb/trunk/tools/driver/Platform.cpp

 #if defined( _WIN32 )
 
 #include <process.h>
 #include <assert.h>
 #include <stdlib.h>
 
 #include "Platform.h"
 
-// the control handler or SIGINT handler
-static sighandler_t _ctrlHandler = NULL;
-
-// the default console control handler
-BOOL
-WINAPI CtrlHandler (DWORD ctrlType)
-{
-    if ( _ctrlHandler != NULL )
-    {
-        _ctrlHandler( 0 );
-        return TRUE;
-    }
-    return FALSE;
-}
-
 int
 ioctl (int d, int request, ...)
 {
     switch ( request )
     {
     // request the console windows size
     case ( TIOCGWINSZ ):
         {

 int
 tcgetattr (int fildes, struct termios *termios_p)
 {
 //  assert( !"Not implemented!" );
     // error return value (0=success)
     return -1;
 }
 
-#ifdef _MSC_VER
-sighandler_t
-signal (int sig, sighandler_t sigFunc)
-{
-    switch ( sig )
-    {
-    case ( SIGINT ):
-        {
-            _ctrlHandler = sigFunc;
-            SetConsoleCtrlHandler( CtrlHandler, TRUE );
-        }
-        break;
-    case ( SIGPIPE  ):
-    case ( SIGWINCH ):
-    case ( SIGTSTP  ):
-    case ( SIGCONT  ):
-        // ignore these for now
-        break;
-    default:
-        assert( !"Not implemented!" );
-    }
-    return 0;
-}
-#endif
-
 #endif

lldb/trunk/tools/lldb-mi/CMakeLists.txt

set(LLDB_MI_SOURCES
   MIUtilParse.cpp
   MIUtilDateTimeStd.cpp
   MIUtilDebug.cpp
   MIUtilFileStd.cpp
   MIUtilMapIdToVariant.cpp
   MIUtilString.cpp
   MIUtilThreadBaseStd.cpp
   MIUtilVariant.cpp
-  Platform.cpp
   )
 
 if ( CMAKE_SYSTEM_NAME MATCHES "Windows" OR CMAKE_SYSTEM_NAME MATCHES "NetBSD" )
   add_definitions( -DIMPORT_LIBLLDB )
   list(APPEND LLDB_MI_SOURCES
     ${LLDB_SOURCE_ROOT}/Host/common/GetOptInc.cpp
     )
 endif ()

lldb/trunk/tools/lldb-mi/Platform.h

 //===-- Platform.h ----------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #pragma once
 
 #if defined(_MSC_VER)
 
-// this will stop signal.h being included
-#define _INC_SIGNAL
-
 #include <io.h>
 #include <eh.h>
 #include <inttypes.h>
+#include <signal.h>
 #include <lldb/Host/windows/Windows.h>
 #include <lldb/Host/HostGetOpt.h>
 
 // This is not used by MI
 struct timeval
 {
     long tv_sec;
     long tv_usec;

 extern int tcsetattr(int fd, int optional_actions, const struct termios *termios_p);
 extern int tcgetattr(int fildes, struct termios *termios_p);
 
 // signal handler function pointer type
 typedef void (*sighandler_t)(int);
 
 // CODETAG_IOR_SIGNALS
 // signal.h
-#define SIGINT 2                   // Terminal interrupt signal
 #define SIGQUIT 3                  // Terminal quit signal
 #define SIGKILL 9                  // Kill (cannot be caught or ignored)
 #define SIGPIPE 13                 // Write on a pipe with no one to read it
 #define SIGCONT 18                 // Continue executing, if stopped.
 #define SIGTSTP 20                 // Terminal stop signal
 #define SIGSTOP 23                 // Stop executing (cannot be caught or ignored)
 #define SIGWINCH 28                // (== SIGVTALRM)
-#define SIG_DFL ((sighandler_t)-1) // Default handler
-#define SIG_IGN ((sighandler_t)-2) // Ignored
-
-extern sighandler_t signal(int sig, sighandler_t);
 
 #else
 
 #include <inttypes.h>
 #include <limits.h>
 
 #include <getopt.h>
 #include <libgen.h>
 #include <sys/ioctl.h>
 #include <termios.h>
 #include <unistd.h>
 
 #include <pthread.h>
 #include <sys/time.h>
 
 #endif

lldb/trunk/tools/lldb-mi/Platform.cpp

-//===-- Platform.cpp --------------------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-// this file is only relevant for Visual C++
-#if defined(_MSC_VER)
-
-#include <process.h>
-#include <assert.h>
-
-#include "Platform.h"
-
-// the control handler or SIGINT handler
-static sighandler_t _ctrlHandler = NULL;
-
-// the default console control handler
-BOOL WINAPI CtrlHandler(DWORD ctrlType)
-{
-    if (_ctrlHandler != NULL)
-    {
-        _ctrlHandler(SIGINT);
-        return TRUE;
-    }
-    return FALSE;
-}
-
-sighandler_t
-signal(int sig, sighandler_t sigFunc)
-{
-    switch (sig)
-    {
-        case (SIGINT):
-        {
-            _ctrlHandler = sigFunc;
-            SetConsoleCtrlHandler(CtrlHandler, TRUE);
-        }
-        break;
-        default:
-            assert(!"Not implemented!");
-    }
-    return 0;
-}
-
-#endif