Setting Thread.Name sets the native thread name on MacOS.

[esoros]

Setting Thread.Name will now set the name of the native thread
on MacOS - due to API constraints this will not work cross thread.

I couldn't find a good way to write automated tests for this (you can't set the name cross thread, but I'm open to ideas :)) but I tested this in lldb and verified that the thread name shows up in the debugger with a quick little C# program that spins up some threads and sets the name.

using System;
using System.Threading;

namespace HelloThread
{
    class Program
    {
        internal static void ThreadStart(object threadName) {
            Thread.CurrentThread.Name = threadName.ToString();    
            for(;;) {
                Console.Out.WriteLine($"Hello, {threadName}");
                Thread.Sleep(1000);
            }
        }
        
        static void Main(string[] args) {
            Thread t1 = new Thread(ThreadStart);
            t1.Start("thread 1");

            Thread t2 = new Thread(ThreadStart);
            t2.Start("thread 2");

            t1.Join();
            t2.Join();
        }
    }
}

I ran the tests locally and was getting some errors, but I saw the same errors when running them against master so I'm wondering what happens with CI.

Fixes #47087

[dnfadmin]

All CLA requirements met.

[am11]

I couldn't find a good way to write automated tests for this

Typically, palsuite is used for testing such functionality, e.g.

runtime/src/coreclr/pal/tests/palsuite/threading/ExitThread/test1/test1.cpp

Line 19 in 0fcfa46

DWORD PALAPI ExitThreadTestThread( LPVOID lpParameter)

[esoros]

I added some automated tests but had to had a new function to the API (GetThreadDescription) - idk if this is cool or not.

[am11]

I think we can delete this function from here and implement it a bit differently in palsuite due to the following reasons:

• some platforms like Alpine Linux and FreeBSD do not have pthread_getname_np (in the versions we are using in CI)
• this API is not used by the product.

then in palsuite, (for now) we can:

• use pthread_self() call pthread_getname_np() on it.
• execute tests only for defined(_APPLE_) || (defined(__linux__) && !defined(TARGET_ALPINE_LINUX))

[esoros]

I had originally tried that (using pthread_getname_np() in the test to read the thread name) but couldn't get that to compile because it looks like there's some type collisions with typedefs between PAL and the MacOS SDK. Idk if there's a good way to get around this or not. I get this when trying to include <palsuite.h> and <pthread.h>

[am11]

Ah true; the include combination of PAL is bit tricky. We just need to separate the implementation in a separate source file: am11@40d7d85 on top of your branch. Tested on Linux and macOS with:

$ ./build.sh clr.paltests
$ ./src/coreclr/pal/tests/palsuite/runpaltests.sh $(pwd)/artifacts/bin/coreclr/OSX.x64.Debug/paltests
# replace OSX.x64.Debug with Linux.x64.Debug for Linux

... snip ...

PAL Test Results:
  Passed: 698 # 695 on macOS
  Failed: 0

[jkotas]

I am not sure sure whether this test is worth all this trouble. It does not test whether the managed API (what we actually care about) works.

[am11]

I think all ~700 PAL tests are no different; testing PAL API surface rather ensuring managed API works?

[esoros]

It looks like pthread_getname_np() isn't implemented in musl (and FreeBSD) so builds that rely on that are still failing; the tests are still building even though they aren't added to the test suite. I couldn't find any #defines that we can use for GetThreadName() so at this point it may be easier just to remove the tests, but I'm open to any other ideas :)

[jkotas]

I think all ~700 PAL tests are no different; testing PAL API surface rather ensuring managed API works?

Most of the PAL tests are testing complex APIs that are not directly exposed as managed API, and the verification is focused on making sure that the API has identical behavior as its Windows equivalent.

[am11]

Yup. On Windows and Linux, Thread.Name_set() updates the name internal to runtime as well as system-wide. On macOS it was only updating the internal name which PR has fixed. Thread.Name_get() returns the internal value on all platforms. This test is ensuring that the name change due to Thread.Name_set() call is reflected on system level, observable by external tools.

Since there is no managed API change, perhaps this system-wide change test is something we don't want to cover. It doesn't hurt IMO. :)

[danmoseley]

Rerunning the leg that failed (due to infra error)

@am11 are you done - this just needs a final review?

[danmoseley]

Oops, it was @esoros working on this of course.

[danmoseley]

@jkotas do you have further feedback?

[jkotas]

LGTM. Thanks!