[EventPipe] Fix linker warning on Debug/Checked build and unlink Unix Domain Socket...
authorJosé Rivero <jorive@microsoft.com>
Thu, 21 Mar 2019 16:08:46 +0000 (09:08 -0700)
committerGitHub <noreply@github.com>
Thu, 21 Mar 2019 16:08:46 +0000 (09:08 -0700)
- Attempt to close-behind unix domain socket and repurpose LF_REMOTING facility.
  - On shutdown, attempt to unlink the bound socket so it can be removed from the file system when the last reference to it it closed.
- Rename unused LF_REMOTING to LF_DIAGNOSTICS_PORT. This new flag will be used by the diagnostic server stress log calls.
- libcmtd.lib(initializers.obj) : warning LNK4098: defaultlib 'libcmt.lib' conflicts with use of other libs; use /NODEFAULTLIB:library [S:\github.com\jorive\coreclr\bin\obj\Windows_NT.x64.Checked\src\dlls\mscoree\coreclr\coreclr.vcxproj]
- Move some preprocessors and includes around.

src/debug/debug-pal/CMakeLists.txt
src/debug/debug-pal/unix/diagnosticsipc.cpp
src/debug/debug-pal/win/diagnosticsipc.cpp
src/debug/inc/diagnosticsipc.h
src/inc/loglf.h
src/vm/diagnosticserver.cpp
src/vm/diagnosticserver.h
src/vm/fastserializer.cpp
src/vm/fastserializer.h

index c1b5b44..ac0f97e 100644 (file)
@@ -5,12 +5,10 @@ include_directories(../../pal/inc)
 add_definitions(-DPAL_STDCPP_COMPAT=1)
 
 if(WIN32)
-    #use static crt
-    add_definitions(-MT) 
     add_definitions(-DWIN32_LEAN_AND_MEAN)
     include_directories(../../inc) #needed for warning control
 
-    set(TWO_WAY_PIPE_SOURCES 
+    set(TWO_WAY_PIPE_SOURCES
         win/diagnosticsipc.cpp
         win/twowaypipe.cpp
         win/processdescriptor.cpp
index 182dff0..d9e0e94 100644 (file)
@@ -7,7 +7,6 @@
 #include <new>
 #include <unistd.h>
 #include <fcntl.h>
-#include <sys/types.h>
 #include <sys/socket.h>
 #include <sys/un.h>
 #include "diagnosticsipc.h"
@@ -15,7 +14,8 @@
 
 IpcStream::DiagnosticsIpc::DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress) :
     _serverSocket(serverSocket),
-    _pServerAddress(new (std::nothrow) sockaddr_un)
+    _pServerAddress(new (std::nothrow) sockaddr_un),
+    _isUnlinked(false)
 {
     _ASSERTE(_pServerAddress != nullptr);
     _ASSERTE(_serverSocket != -1);
@@ -33,8 +33,7 @@ IpcStream::DiagnosticsIpc::~DiagnosticsIpc()
         const int fSuccessClose = ::close(_serverSocket);
         _ASSERTE(fSuccessClose != -1); // TODO: Add error handling?
 
-        const int fSuccessUnlink = ::unlink(_pServerAddress->sun_path);
-        _ASSERTE(fSuccessUnlink != -1); // TODO: Add error handling?
+        Unlink();
 
         delete _pServerAddress;
     }
@@ -68,6 +67,26 @@ IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const p
         if (callback != nullptr)
             callback(strerror(errno), errno);
         _ASSERTE(fSuccessBind != -1);
+
+        const int fSuccessClose = ::close(serverSocket);
+        _ASSERTE(fSuccessClose != -1);
+
+        return nullptr;
+    }
+
+    const int fSuccessfulListen = ::listen(serverSocket, /* backlog */ 255);
+    if (fSuccessfulListen == -1)
+    {
+        if (callback != nullptr)
+            callback(strerror(errno), errno);
+        _ASSERTE(fSuccessfulListen != -1);
+
+        const int fSuccessUnlink = ::unlink(serverAddress.sun_path);
+        _ASSERTE(fSuccessUnlink != -1);
+
+        const int fSuccessClose = ::close(serverSocket);
+        _ASSERTE(fSuccessClose != -1);
+
         return nullptr;
     }
 
@@ -76,8 +95,6 @@ IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const p
 
 IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const
 {
-    if (::listen(_serverSocket, /* backlog */ 255) == -1)
-        return nullptr;
     sockaddr_un from;
     socklen_t fromlen = sizeof(from);
     const int clientSocket = ::accept(_serverSocket, (sockaddr *)&from, &fromlen);
@@ -94,6 +111,28 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const
     return pIpcStream;
 }
 
+//! This helps remove the socket from the filesystem when the runtime exits.
+//! From: http://man7.org/linux/man-pages/man7/unix.7.html#NOTES
+//!   Binding to a socket with a filename creates a socket in the
+//!   filesystem that must be deleted by the caller when it is no longer
+//!   needed (using unlink(2)).  The usual UNIX close-behind semantics
+//!   apply; the socket can be unlinked at any time and will be finally
+//!   removed from the filesystem when the last reference to it is closed.
+void IpcStream::DiagnosticsIpc::Unlink(ErrorCallback callback)
+{
+    if (_isUnlinked)
+        return;
+    _isUnlinked = true;
+
+    const int fSuccessUnlink = ::unlink(_pServerAddress->sun_path);
+    if (fSuccessUnlink == -1)
+    {
+        if (callback != nullptr)
+            callback(strerror(errno), errno);
+        _ASSERTE(fSuccessUnlink == 0);
+    }
+}
+
 IpcStream::~IpcStream()
 {
     if (_clientSocket != -1)
index 7d9f84e..46f2128 100644 (file)
@@ -77,6 +77,10 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const
     return pIpcStream;
 }
 
+void IpcStream::DiagnosticsIpc::Unlink(ErrorCallback callback)
+{
+}
+
 IpcStream::~IpcStream()
 {
     if (_hPipe != INVALID_HANDLE_VALUE)
index 81789f0..54277e1 100644 (file)
@@ -26,20 +26,31 @@ public:
     class DiagnosticsIpc final
     {
     public:
-        static DiagnosticsIpc *Create(const char *const pIpcName, ErrorCallback callback = nullptr);
         ~DiagnosticsIpc();
+
+        //! Creates an IPC object
+        static DiagnosticsIpc *Create(const char *const pIpcName, ErrorCallback callback = nullptr);
+
+        //! Enables the underlaying IPC implementation to accept connection.
         IpcStream *Accept(ErrorCallback callback = nullptr) const;
 
+        //! Used to unlink the socket so it can be removed from the filesystem
+        //! when the last reference to it is closed.
+        void Unlink(ErrorCallback callback = nullptr);
+
     private:
 
 #ifdef FEATURE_PAL
-        DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress);
-        const int _serverSocket = -1;
+        const int _serverSocket;
         sockaddr_un *const _pServerAddress;
+        bool _isUnlinked = false;
+
+        DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress);
 #else
         static const uint32_t MaxNamedPipeNameLength = 256;
-        DiagnosticsIpc(const char(&namedPipeName)[MaxNamedPipeNameLength]);
         char _pNamedPipeName[MaxNamedPipeNameLength]; // https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-createnamedpipea
+
+        DiagnosticsIpc(const char(&namedPipeName)[MaxNamedPipeNameLength]);
 #endif /* FEATURE_PAL */
 
         DiagnosticsIpc() = delete;
index e7fbd51..76a9897 100644 (file)
@@ -16,7 +16,7 @@ DEFINE_LOG_FACILITY(LF_GCALLOC           ,0x00000100)
 DEFINE_LOG_FACILITY(LF_CORDB             ,0x00000200)
 DEFINE_LOG_FACILITY(LF_CLASSLOADER       ,0x00000400)
 DEFINE_LOG_FACILITY(LF_CORPROF           ,0x00000800)
-DEFINE_LOG_FACILITY(LF_REMOTING          ,0x00001000)
+DEFINE_LOG_FACILITY(LF_DIAGNOSTICS_PORT  ,0x00001000)
 DEFINE_LOG_FACILITY(LF_DBGALLOC          ,0x00002000)
 DEFINE_LOG_FACILITY(LF_EH                ,0x00004000)
 DEFINE_LOG_FACILITY(LF_ENC               ,0x00008000)
index c0e11d1..3a70038 100644 (file)
@@ -4,7 +4,6 @@
 
 #include "common.h"
 #include "diagnosticserver.h"
-#include "diagnosticsipc.h"
 #include "eventpipeprotocolhelper.h"
 
 #ifdef FEATURE_PAL
@@ -13,6 +12,8 @@
 
 #ifdef FEATURE_PERFTRACING
 
+IpcStream::DiagnosticsIpc *DiagnosticServer::s_pIpc = nullptr;
+
 static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter)
 {
     CONTRACTL
@@ -28,17 +29,13 @@ static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter)
     auto pIpc = reinterpret_cast<IpcStream::DiagnosticsIpc *>(lpThreadParameter);
     if (pIpc == nullptr)
     {
-        STRESS_LOG0(LF_STARTUP, LL_ERROR,"Diagnostics IPC listener was undefined\n");
+        STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ERROR, "Diagnostics IPC listener was undefined\n");
         return 1;
     }
 
-#ifdef _DEBUG
     ErrorCallback LoggingCallback = [](const char *szMessage, uint32_t code) {
-        LOG((LF_REMOTING, LL_WARNING, "warning (%d): %s.\n", code, szMessage));
+        STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_WARNING, "warning (%d): %s.\n", code, szMessage);
     };
-#else
-    ErrorCallback LoggingCallback = nullptr;
-#endif
 
     while (true)
     {
@@ -69,7 +66,7 @@ static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter)
             break;
 
         default:
-            LOG((LF_REMOTING, LL_WARNING, "Received unknow request type (%d)\n", header.RequestType));
+            LOG((LF_DIAGNOSTICS_PORT, LL_WARNING, "Received unknow request type (%d)\n", header.RequestType));
             break;
         }
     }
@@ -93,23 +90,25 @@ bool DiagnosticServer::Initialize()
     {
         auto ErrorCallback = [](const char *szMessage, uint32_t code) {
             STRESS_LOG2(
-                LF_STARTUP,                                           // facility
+                LF_DIAGNOSTICS_PORT,                                  // facility
                 LL_ERROR,                                             // level
                 "Failed to create diagnostic IPC: error (%d): %s.\n", // msg
                 code,                                                 // data1
                 szMessage);                                           // data2
         };
-        IpcStream::DiagnosticsIpc *pIpc = IpcStream::DiagnosticsIpc::Create(
+
+        // TODO: Should we handle/assert that (s_pIpc == nullptr)?
+        s_pIpc = IpcStream::DiagnosticsIpc::Create(
             "dotnetcore-diagnostic", ErrorCallback);
 
-        if (pIpc != nullptr)
+        if (s_pIpc != nullptr)
         {
             DWORD dwThreadId = 0;
             HANDLE hThread = ::CreateThread( // TODO: Is it correct to have this "lower" level call here?
                 nullptr,                     // no security attribute
                 0,                           // default stack size
                 DiagnosticsServerThread,     // thread proc
-                (LPVOID)pIpc,                // thread parameter
+                (LPVOID)s_pIpc,              // thread parameter
                 0,                           // not suspended
                 &dwThreadId);                // returns thread ID
 
@@ -117,7 +116,7 @@ bool DiagnosticServer::Initialize()
             {
                 // Failed to create IPC thread.
                 STRESS_LOG1(
-                    LF_STARTUP,                                          // facility
+                    LF_DIAGNOSTICS_PORT,                                 // facility
                     LL_ERROR,                                            // level
                     "Failed to create diagnostic server thread (%d).\n", // msg
                     ::GetLastError());                                   // data1
@@ -155,7 +154,18 @@ bool DiagnosticServer::Shutdown()
 
     EX_TRY
     {
-        // FIXME: Stop IPC server thread?
+        if (s_pIpc != nullptr)
+        {
+            auto ErrorCallback = [](const char *szMessage, uint32_t code) {
+                STRESS_LOG2(
+                    LF_DIAGNOSTICS_PORT,                                  // facility
+                    LL_ERROR,                                             // level
+                    "Failed to unlink diagnostic IPC: error (%d): %s.\n", // msg
+                    code,                                                 // data1
+                    szMessage);                                           // data2
+            };
+            s_pIpc->Unlink(ErrorCallback);
+        }
         fSuccess = true;
     }
     EX_CATCH
index 95399a2..51f32ae 100644 (file)
@@ -5,10 +5,11 @@
 #ifndef __DIAGNOSTIC_SERVER_H__
 #define __DIAGNOSTIC_SERVER_H__
 
-#include <stdint.h>
-
 #ifdef FEATURE_PERFTRACING // This macro should change to something more generic than performance.
 
+#include <stdint.h>
+#include "diagnosticsipc.h"
+
 //! TODO: Temp class.
 enum class DiagnosticMessageType : uint32_t
 {
@@ -51,6 +52,9 @@ public:
 
     //! Shutdown the event pipe.
     static bool Shutdown();
+
+private:
+    static IpcStream::DiagnosticsIpc *s_pIpc;
 };
 
 #endif // FEATURE_PERFTRACING
index 89dbe50..e02ec93 100644 (file)
@@ -4,6 +4,7 @@
 
 #include "common.h"
 #include "fastserializer.h"
+#include "diagnosticsipc.h"
 
 #ifdef FEATURE_PERFTRACING
 
index 3b4de65..98208c0 100644 (file)
@@ -11,7 +11,8 @@
 
 #include "fastserializableobject.h"
 #include "fstream.h"
-#include "diagnosticsipc.h"
+
+class IpcStream;
 
 // the enumeration has a specific set of values to keep it compatible with consumer library
 // it's sibling is defined in https://github.com/Microsoft/perfview/blob/10d1f92b242c98073b3817ac5ee6d98cd595d39b/src/FastSerialization/FastSerialization.cs#L2295