Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Implement SetThreadName in PAL for Linux. #27182

Merged
merged 4 commits into from
Nov 11, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/pal/inc/pal.h
Original file line number Diff line number Diff line change
Expand Up @@ -2410,6 +2410,14 @@ GetThreadTimes(
OUT LPFILETIME lpKernelTime,
OUT LPFILETIME lpUserTime);

PALIMPORT
HRESULT
PALAPI
SetThreadName(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual Windows API is named SetThreadDescription and the SetThreadName is just a wrapper to this function defined in src/utilcode/winfix.cpp. The PAL either implements Windows APIs 1:1 or use PAL_ prefix in for names of functions that are not Windows APIs.
Can you please rename this function to SetThreadDescription and add a wrapper function named SetThreadName that just calls this function? You can put it at the end of the winfix.cpp file (add #else // !FEATURE_PAL before the #endif and put the wrapper there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the result is ignored by the calling code anyway. Since this is just a diagnostic aid API, it may be best to just make a PAL_ function that returns void.

IN HANDLE hThread,
IN PCWSTR lpThreadDescription
);

#define TLS_OUT_OF_INDEXES ((DWORD)0xFFFFFFFF)

PALIMPORT
Expand Down
8 changes: 8 additions & 0 deletions src/pal/src/include/pal/thread.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,14 @@ namespace CorUnix
int
);

friend
PAL_ERROR
InternalSetThreadName(
CPalThread *,
HANDLE,
PCWSTR
);

friend
PAL_ERROR
CreateThreadData(
Expand Down
110 changes: 110 additions & 0 deletions src/pal/src/thread/thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1578,7 +1578,117 @@ GetThreadTimes(
return (retval);
}

HRESULT
PALAPI
SetThreadName(
IN HANDLE hThread,
IN PCWSTR lpThreadDescription)
{
CPalThread *pThread;
PAL_ERROR palError;

PERF_ENTRY(SetThreadName);
ENTRY("SetThreadName(hThread=%p,lpThreadDescription=%p)\n", hThread, lpThreadDescription);

pThread = InternalGetCurrentThread();

palError = InternalSetThreadName(
pThread,
hThread,
lpThreadDescription
);

if (NO_ERROR != palError)
{
pThread->SetLastError(palError);
}

LOGEXIT("SetThreadNameExit");
PERF_EXIT(SetThreadName);

return NO_ERROR != palError ? 0x80004005 : 0; // E_FAIL
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure how to correctly get a hold of the HRESULT macros, or if I should even return a bog standard one like E_FAIL here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we don't use HRESULT anywhere in PAL, so the macros to translate between win32 error (which is the same as PAL error) and HRESULT are not available. We have a couple of options:

  1. Move the
    /******************* HRESULT types ****************************************/
    #define FACILITY_WINDOWS 8
    #define FACILITY_URT 19
    #define FACILITY_UMI 22
    #define FACILITY_SXS 23
    #define FACILITY_STORAGE 3
    #define FACILITY_SSPI 9
    #define FACILITY_SCARD 16
    #define FACILITY_SETUPAPI 15
    #define FACILITY_SECURITY 9
    #define FACILITY_RPC 1
    #define FACILITY_WIN32 7
    #define FACILITY_CONTROL 10
    #define FACILITY_NULL 0
    #define FACILITY_MSMQ 14
    #define FACILITY_MEDIASERVER 13
    #define FACILITY_INTERNET 12
    #define FACILITY_ITF 4
    #define FACILITY_DPLAY 21
    #define FACILITY_DISPATCH 2
    #define FACILITY_COMPLUS 17
    #define FACILITY_CERT 11
    #define FACILITY_ACS 20
    #define FACILITY_AAF 18
    #define NO_ERROR 0L
    #define SEVERITY_SUCCESS 0
    #define SEVERITY_ERROR 1
    #define SUCCEEDED(Status) ((HRESULT)(Status) >= 0)
    #define FAILED(Status) ((HRESULT)(Status)<0)
    #define IS_ERROR(Status) ((ULONG)(Status) >> 31 == SEVERITY_ERROR) // diff from win32
    #define HRESULT_CODE(hr) ((hr) & 0xFFFF)
    #define SCODE_CODE(sc) ((sc) & 0xFFFF)
    #define HRESULT_FACILITY(hr) (((hr) >> 16) & 0x1fff)
    #define SCODE_FACILITY(sc) (((sc) >> 16) & 0x1fff)
    #define HRESULT_SEVERITY(hr) (((hr) >> 31) & 0x1)
    #define SCODE_SEVERITY(sc) (((sc) >> 31) & 0x1)
    // both macros diff from Win32
    #define MAKE_HRESULT(sev,fac,code) \
    ((HRESULT) (((ULONG)(sev)<<31) | ((ULONG)(fac)<<16) | ((ULONG)(code))) )
    #define MAKE_SCODE(sev,fac,code) \
    ((SCODE) (((ULONG)(sev)<<31) | ((ULONG)(fac)<<16) | ((LONG)(code))) )
    #define FACILITY_NT_BIT 0x10000000
    #define HRESULT_FROM_WIN32(x) ((HRESULT)(x) <= 0 ? ((HRESULT)(x)) : ((HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000)))
    #define __HRESULT_FROM_WIN32(x) HRESULT_FROM_WIN32(x)
    #define HRESULT_FROM_NT(x) ((HRESULT) ((x) | FACILITY_NT_BIT))
    to the pal.h, which would allow you to use the HRESULT_FROM_WIN32(palError) here.
  2. Change the function to return PAL_ERROR instead of HRESULT and do the translation to HRESULT in the wrapper function, I've mentioned in my previous comment. In that case, we should rename the function to have a PAL_ prefix to indicate it is not a 1:1 implementation of the Windows API.

I would prefer option 1

}

PAL_ERROR
CorUnix::InternalSetThreadName(
CPalThread *pThread,
HANDLE hTargetThread,
PCWSTR lpThreadDescription
)
{
PAL_ERROR palError = NO_ERROR;
CPalThread *pTargetThread = NULL;
IPalObject *pobjThread = NULL;
int nameSize;
char *nameBuf = NULL;
int error;

// The exact API of pthread_setname_np varies very wildly depending on OS.
// For now, only Linux is implemented.
#if defined(__linux__)

palError = InternalGetThreadDataFromHandle(
pThread,
hTargetThread,
&pTargetThread,
&pobjThread
);

if (NO_ERROR != palError)
{
goto InternalSetThreadNameExit;
}

pTargetThread->Lock(pThread);

/* translate the wide char lpPathName string to multibyte string */
if(0 == (nameSize = WideCharToMultiByte( CP_ACP, 0, lpThreadDescription, -1, NULL, 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer moving the nameSize assignment before the if. The same for the nameBuf below. It makes it easier to read and step through the code during debugging.

NULL, NULL )))
{
palError = ERROR_INTERNAL_ERROR;
goto InternalSetThreadNameExit;
}

if (((nameBuf = (char *)PAL_malloc(nameSize)) == NULL) ||
(WideCharToMultiByte( CP_ACP, 0, lpThreadDescription, -1, nameBuf, nameSize, NULL,
NULL) != nameSize))
{
palError = ERROR_INTERNAL_ERROR;
goto InternalSetThreadNameExit;
}

// Null terminate early.
// pthread_setname_np only accepts up to 16 chars.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do names of all threads that the runtime look reasonable when they are cutoff to 15 characters?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This can be tweaked in follow up change.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Threadpool and finalizer threads at least are recognizable from my testing.

nameBuf[15] = '\0';

error = pthread_setname_np(pTargetThread->GetPThreadSelf(), nameBuf);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On FreeBSD, this is pthread_set_name_np() (and requires <pthread_np.h>). On macOS, this is pthread_setname_np(), only the thread name is passed as an argument (setting the name for the calling thread); it also takes a char* rather than a const char*, so I often copy it on the stack with strdupa() when calling it.

No need to implement all these flavors in this PR, but these are all the differences as far as I can tell, so the #ifdef __linux__ block could encase just this pthread call for all the supported platforms. (Maybe with a #else returning some "not supported" PAL_ERROR.) Maybe as a followup patch?


if (error != 0)
{
palError = ERROR_INTERNAL_ERROR;
goto InternalSetThreadNameExit;
}

InternalSetThreadNameExit:

if (NULL != pTargetThread)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pTargetThread won't be NULL here (if it is, then the call to lock will blow up).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it possible that InternalGetThreadDataFromHandle up above will fail and then keep the pTargetThread pointer NULL? If that were to happen this check would be necessary.

{
pTargetThread->Unlock(pThread);
}

if (NULL != pobjThread)
{
pobjThread->ReleaseReference(pThread);
}

if (NULL != nameBuf) {
PAL_free(nameBuf);
}

#endif // defined(__linux__)

return palError;
}

void *
CPalThread::ThreadEntry(
Expand Down
3 changes: 1 addition & 2 deletions src/vm/comsynchronizable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1283,14 +1283,13 @@ void QCALLTYPE ThreadNative::InformThreadNameChange(QCall::ThreadHandle thread,

Thread* pThread = &(*thread);

#ifndef FEATURE_PAL
// Set on Windows 10 Creators Update and later machines the unmanaged thread name as well. That will show up in ETW traces and debuggers which is very helpful
// if more and more threads get a meaningful name
// Will also show up in Linux in gdb and such.
if (len > 0 && name != NULL && pThread->GetThreadHandle() != INVALID_HANDLE_VALUE)
{
SetThreadName(pThread->GetThreadHandle(), name);
}
#endif

#ifdef PROFILING_SUPPORTED
{
Expand Down
5 changes: 2 additions & 3 deletions src/vm/threads.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2071,9 +2071,9 @@ BOOL Thread::CreateNewThread(SIZE_T stackSize, LPTHREAD_START_ROUTINE start, voi
bRet = CreateNewOSThread(stackSize, start, args);
#ifndef FEATURE_PAL
UndoRevert(bReverted, token);
#endif // !FEATURE_PAL
if (pName != NULL)
SetThreadName(m_ThreadHandle, pName);
#endif // !FEATURE_PAL

return bRet;
}
Expand Down Expand Up @@ -2130,9 +2130,8 @@ HANDLE Thread::CreateUtilityThread(Thread::StackSizeBucket stackSizeBucket, LPTH

DWORD threadId;
HANDLE hThread = CreateThread(NULL, stackSize, start, args, flags, &threadId);
#ifndef FEATURE_PAL

SetThreadName(hThread, pName);
#endif // !FEATURE_PAL


if (pThreadId)
Expand Down
3 changes: 1 addition & 2 deletions src/vm/win32threadpool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1824,9 +1824,8 @@ Thread* ThreadpoolMgr::CreateUnimpersonatedThread(LPTHREAD_START_ROUTINE lpStart
lpThreadArgs, // arguments
CREATE_SUSPENDED,
&threadId);
#ifndef FEATURE_PAL

SetThreadName(threadHandle, W(".NET ThreadPool Worker"));
#endif // !FEATURE_PAL
if (threadHandle != NULL)
lpThreadArgs.SuppressRelease();
}
Expand Down