Implement SetThreadName in PAL for Linux. #27182

PJB3005 · 2019-10-14T20:58:27Z

This means thread names will now show up in gdb, htop, etc... on Linux.

I did not implement this for any other platforms:
I did not have anything to test them with, and pthread_setname_np's API varies wildly across platform. (Hell, on macOS you can't call it from any thread except the thread in question..)

I'm not very familiar with C/C++ and this is my first time contributing, so extra scrutiny would be very much appreciated.

This means thread names will now show up in gdb, htop, etc... on Linux. I did not implement this for any other platforms: I did not have anything to test them with, and pthread_setname_np's API varies wildly.

dnfclas · 2019-10-14T20:58:42Z

All CLA requirements met.

PJB3005 · 2019-10-15T08:28:29Z

src/pal/src/thread/thread.cpp

+    LOGEXIT("SetThreadNameExit");
+    PERF_EXIT(SetThreadName);
+
+    return NO_ERROR != palError ? 0x80004005 : 0; // E_FAIL


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.

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:

Move the

coreclr/src/pal/inc/rt/palrt.h

Lines 321 to 373 in 921b39c

/******************* 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.

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

janvorli · 2019-10-16T14:16:53Z

src/pal/inc/pal.h

+PALIMPORT
+HRESULT
+PALAPI
+SetThreadName(


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.

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.

src/pal/src/thread/thread.cpp

janvorli · 2019-10-16T15:27:50Z

src/pal/src/thread/thread.cpp

+    pTargetThread->Lock(pThread);
+
+    /* translate the wide char lpPathName string to multibyte string */
+    if(0 == (nameSize = WideCharToMultiByte( CP_ACP, 0, lpThreadDescription, -1, NULL, 0,


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.

janvorli · 2019-10-16T16:41:39Z

src/pal/src/thread/thread.cpp

+    LOGEXIT("SetThreadNameExit");
+    PERF_EXIT(SetThreadName);
+
+    return NO_ERROR != palError ? 0x80004005 : 0; // E_FAIL


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:

Move the

coreclr/src/pal/inc/rt/palrt.h

Lines 321 to 373 in 921b39c

/******************* 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.

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

src/pal/src/thread/thread.cpp

jkotas · 2019-10-25T16:59:23Z

src/pal/src/thread/thread.cpp

+    }
+
+    // Null terminate early.
+    // pthread_setname_np only accepts up to 16 chars.


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

(This can be tweaked in follow up change.)

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

jkotas

LGTM. Thank you!

jkotas · 2019-10-25T17:00:32Z

Build breaks on Mac:

Undefined symbols for architecture x86_64:
"_SetThreadDescription", referenced from:
SetThreadName(void*, char16_t const*) in libutilcodestaticnohost.a(winfix.cpp.o)

jkotas

Could you please take a look at the Mac build breaks?

lpereira · 2019-10-25T17:24:19Z

src/pal/src/thread/thread.cpp

+    // pthread_setname_np only accepts up to 16 chars.
+    nameBuf[15] = '\0';
+
+    error = pthread_setname_np(pTargetThread->GetPThreadSelf(), nameBuf);


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?

lpereira · 2019-10-25T17:26:25Z

src/pal/src/thread/thread.cpp

+    if (error != 0) 
+    {
+        palError = ERROR_INTERNAL_ERROR;
+        goto InternalSetThreadDescriptionExit;


This goto can be removed (it jumps to the next line).

lpereira · 2019-10-25T17:27:47Z

src/pal/src/thread/thread.cpp

+
+InternalSetThreadDescriptionExit:
+
+    if (NULL != pTargetThread)


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

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.

src/pal/src/thread/thread.cpp

janvorli · 2019-11-05T19:22:09Z

@PJB3005 can you please take a look at the Mac build breaks, as @jkotas has mentioned about 10 days ago? Since we will be migrating to a new repo that will share coreclr, corefx and core-setup stuff, we have about a week large window for merging this change here, otherwise it would have to be ported to the new repo.

PJB3005 · 2019-11-06T11:37:54Z

Oh jeez it's already been 10 days.

I don't really have a mac that I can test this on, and I can't figure out anything just looking at the build logs.

maryamariyan · 2019-11-06T21:03:42Z

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

In your coreclr repository clone, create patch by running git format-patch origin
In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

janvorli · 2019-11-08T16:38:34Z

I don't really have a mac that I can test this on, and I can't figure out anything just looking at the build logs.

Ok, let me try it on my Mac

janvorli · 2019-11-08T17:34:45Z

@PJB3005 I've found the OSX build break issue. The SetThreadDescription needs to be added after the following line, prefixed with #:

coreclr/src/dlls/mscordac/mscordac_unixexports.src

Line 172 in 69a5702

#SetErrorMode

The libmscordbi.dylib depends on libmscordac.dylib exporting some of the PAL symbols so that it can use the PAL linked into the libmscordac.dylib.

PJB3005 · 2019-11-10T10:01:02Z

That seems to have worked, thanks!

@janvorli

… Linux Fixes dotnet/runtime#33673 This issue is a side-effect of adding support for setting thread names on Linux in dotnet/coreclr#27182. cc @janvorli @lpereira @SteveL-MSFT

… Linux (#34064) Fixes #33673 This issue is a side-effect of adding support for setting thread names on Linux in dotnet/coreclr#27182. Co-authored-by: Dan Moseley <[email protected]> Co-authored-by: Aleksey Kliger (λgeek) <[email protected]>

Implement SetThreadName in PAL for Linux.

0d7ba24

This means thread names will now show up in gdb, htop, etc... on Linux. I did not implement this for any other platforms: I did not have anything to test them with, and pthread_setname_np's API varies wildly.

AaronRobinsonMSFT requested a review from janvorli October 15, 2019 01:12

AaronRobinsonMSFT added the area-PAL label Oct 15, 2019

PJB3005 commented Oct 15, 2019

View reviewed changes

janvorli reviewed Oct 16, 2019

View reviewed changes

Requested changes.

aae69a9

jkotas reviewed Oct 25, 2019

View reviewed changes

jkotas approved these changes Oct 25, 2019

View reviewed changes

jkotas suggested changes Oct 25, 2019

View reviewed changes

lpereira reviewed Oct 25, 2019

View reviewed changes

src/pal/src/thread/thread.cpp Show resolved Hide resolved

Remove unecessary goto.

8b7567e

Fix mac builds hopefully

a570aca

janvorli merged commit 5f5f960 into dotnet:master Nov 11, 2019

PJB3005 deleted the 19-10-14-pthread_names branch November 12, 2019 00:51

tmds mentioned this pull request Mar 17, 2020

System.Diagnostics.Process.ProcessName returning incorrect information on Linux with .NET 5 dotnet/runtime#33673

Closed

tmds mentioned this pull request Mar 25, 2020

Ensure Process.ProcessName doesn't change when setting Thread.Name on Linux dotnet/runtime#34064

Merged

jkotas mentioned this pull request Mar 30, 2020

Runtime thread names dotnet/runtime#34276

Closed

monojenkins mentioned this pull request May 5, 2020

Ensure Process.ProcessName doesn't change when setting Thread.Name on Linux mono/mono#19706

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SetThreadName in PAL for Linux. #27182

Implement SetThreadName in PAL for Linux. #27182

PJB3005 commented Oct 14, 2019

dnfclas commented Oct 14, 2019 •

edited

Loading

PJB3005 Oct 15, 2019

janvorli Oct 16, 2019

janvorli Oct 16, 2019

jkotas Oct 16, 2019

janvorli Oct 16, 2019

janvorli Oct 16, 2019

jkotas Oct 25, 2019

jkotas Oct 25, 2019

PJB3005 Oct 27, 2019

jkotas left a comment

jkotas commented Oct 25, 2019

jkotas left a comment

lpereira Oct 25, 2019

lpereira Oct 25, 2019

lpereira Oct 25, 2019

PJB3005 Nov 6, 2019

janvorli commented Nov 5, 2019

PJB3005 commented Nov 6, 2019

maryamariyan commented Nov 6, 2019

janvorli commented Nov 8, 2019

janvorli commented Nov 8, 2019

PJB3005 commented Nov 10, 2019

	/***************** 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))

Implement SetThreadName in PAL for Linux. #27182

Implement SetThreadName in PAL for Linux. #27182

Conversation

PJB3005 commented Oct 14, 2019

dnfclas commented Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

jkotas commented Oct 25, 2019

jkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janvorli commented Nov 5, 2019

PJB3005 commented Nov 6, 2019

maryamariyan commented Nov 6, 2019

janvorli commented Nov 8, 2019

janvorli commented Nov 8, 2019

PJB3005 commented Nov 10, 2019

dnfclas commented Oct 14, 2019 •

edited

Loading