* [PATCH] win32: set threads name
@ 2022-09-29 13:41 marcandre.lureau
2022-09-29 17:47 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: marcandre.lureau @ 2022-09-29 13:41 UTC (permalink / raw)
To: qemu-devel; +Cc: bin.meng, Marc-André Lureau, Stefan Weil
From: Marc-André Lureau <marcandre.lureau@redhat.com>
As described in:
https://learn.microsoft.com/en-us/visualstudio/debugger/how-to-set-a-thread-name-in-native-code?view=vs-2022
SetThreadDescription() is available since Windows 10, version 1607 and
in some versions only by "Run Time Dynamic Linking". Its declaration is
not yet in mingw, so we lookup the function the same way glib does.
Tested with Visual Studio Community 2022 debugger.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
util/qemu-thread-win32.c | 54 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 53 insertions(+), 1 deletion(-)
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index a2d5a6e825..9861ec5b89 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -19,12 +19,40 @@
static bool name_threads;
+typedef HRESULT (WINAPI *pSetThreadDescription) (HANDLE hThread,
+ PCWSTR lpThreadDescription);
+static pSetThreadDescription SetThreadDescriptionFunc = NULL;
+static HMODULE kernel32_module = NULL;
+
+static bool
+load_set_thread_description(void)
+{
+ static gsize _init_once = 0;
+
+ if (g_once_init_enter(&_init_once)) {
+ kernel32_module = LoadLibraryW(L"kernel32.dll");
+ if (kernel32_module) {
+ SetThreadDescriptionFunc =
+ (pSetThreadDescription)GetProcAddress(kernel32_module,
+ "SetThreadDescription");
+ if (!SetThreadDescriptionFunc) {
+ FreeLibrary(kernel32_module);
+ }
+ }
+ g_once_init_leave(&_init_once, 1);
+ }
+
+ return !!SetThreadDescriptionFunc;
+}
+
void qemu_thread_naming(bool enable)
{
/* But note we don't actually name them on Windows yet */
name_threads = enable;
- fprintf(stderr, "qemu: thread naming not supported on this host\n");
+ if (enable && !load_set_thread_description()) {
+ fprintf(stderr, "qemu: thread naming not supported on this host\n");
+ }
}
static void error_exit(int err, const char *msg)
@@ -400,6 +428,26 @@ void *qemu_thread_join(QemuThread *thread)
return ret;
}
+static bool
+set_thread_description(HANDLE h, const char *name)
+{
+ HRESULT hr;
+ g_autofree wchar_t *namew = NULL;
+
+ if (!load_set_thread_description() || !name) {
+ return false;
+ }
+
+ namew = g_utf8_to_utf16(name, -1, NULL, NULL, NULL);
+ if (!namew) {
+ return false;
+ }
+
+ hr = SetThreadDescriptionFunc(h, namew);
+
+ return SUCCEEDED(hr);
+}
+
void qemu_thread_create(QemuThread *thread, const char *name,
void *(*start_routine)(void *),
void *arg, int mode)
@@ -423,7 +471,11 @@ void qemu_thread_create(QemuThread *thread, const char *name,
if (!hThread) {
error_exit(GetLastError(), __func__);
}
+ if (name_threads) {
+ set_thread_description(hThread, name);
+ }
CloseHandle(hThread);
+
thread->data = data;
}
--
2.37.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] win32: set threads name
2022-09-29 13:41 [PATCH] win32: set threads name marcandre.lureau
@ 2022-09-29 17:47 ` Richard Henderson
2022-09-30 8:08 ` Marc-André Lureau
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2022-09-29 17:47 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: bin.meng, Stefan Weil
On 9/29/22 06:41, marcandre.lureau@redhat.com wrote:
> void qemu_thread_naming(bool enable)
> {
> /* But note we don't actually name them on Windows yet */
> name_threads = enable;
>
> - fprintf(stderr, "qemu: thread naming not supported on this host\n");
> + if (enable && !load_set_thread_description()) {
> + fprintf(stderr, "qemu: thread naming not supported on this host\n");
> + }
> }
Comment is out of date, and I think it would be better to *not* set name_threads if not
supported, rather than...
> +static bool
> +set_thread_description(HANDLE h, const char *name)
> +{
> + HRESULT hr;
> + g_autofree wchar_t *namew = NULL;
> +
> + if (!load_set_thread_description() || !name) {
> + return false;
> + }
... have to re-query load_set_thread_description later.
Also, unused return value; might as well be void.
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] win32: set threads name
2022-09-29 17:47 ` Richard Henderson
@ 2022-09-30 8:08 ` Marc-André Lureau
2022-09-30 13:34 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Marc-André Lureau @ 2022-09-30 8:08 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, bin.meng, Stefan Weil
[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]
Hi
On Thu, Sep 29, 2022 at 9:53 PM Richard Henderson <
richard.henderson@linaro.org> wrote:
> On 9/29/22 06:41, marcandre.lureau@redhat.com wrote:
> > void qemu_thread_naming(bool enable)
> > {
> > /* But note we don't actually name them on Windows yet */
> > name_threads = enable;
> >
> > - fprintf(stderr, "qemu: thread naming not supported on this host\n");
> > + if (enable && !load_set_thread_description()) {
> > + fprintf(stderr, "qemu: thread naming not supported on this
> host\n");
> > + }
> > }
>
> Comment is out of date, and I think it would be better to *not* set
> name_threads if not
> supported, rather than...
>
Comment removed.
>
>
> > +static bool
> > +set_thread_description(HANDLE h, const char *name)
> > +{
> > + HRESULT hr;
> > + g_autofree wchar_t *namew = NULL;
> > +
> > + if (!load_set_thread_description() || !name) {
> > + return false;
> > + }
>
> ... have to re-query load_set_thread_description later.
>
The load_set_thread_description() function is actually a "one-time"
function, it doesn't re-load.
>
> Also, unused return value; might as well be void.
>
Right, maybe it should warn if it failed to set the name?
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 2277 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] win32: set threads name
2022-09-30 8:08 ` Marc-André Lureau
@ 2022-09-30 13:34 ` Richard Henderson
2022-09-30 13:45 ` Marc-André Lureau
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2022-09-30 13:34 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel, bin.meng, Stefan Weil
On 9/30/22 01:08, Marc-André Lureau wrote:
> Hi
>
> On Thu, Sep 29, 2022 at 9:53 PM Richard Henderson <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>> wrote:
>
> On 9/29/22 06:41, marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com> wrote:
> > void qemu_thread_naming(bool enable)
> > {
> > /* But note we don't actually name them on Windows yet */
> > name_threads = enable;
> >
> > - fprintf(stderr, "qemu: thread naming not supported on this host\n");
> > + if (enable && !load_set_thread_description()) {
> > + fprintf(stderr, "qemu: thread naming not supported on this host\n");
> > + }
> > }
>
> Comment is out of date, and I think it would be better to *not* set name_threads if not
> supported, rather than...
>
>
> Comment removed.
>
>
>
> > +static bool
> > +set_thread_description(HANDLE h, const char *name)
> > +{
> > + HRESULT hr;
> > + g_autofree wchar_t *namew = NULL;
> > +
> > + if (!load_set_thread_description() || !name) {
> > + return false;
> > + }
>
> ... have to re-query load_set_thread_description later.
>
>
> The load_set_thread_description() function is actually a "one-time" function, it doesn't
> re-load.
You're calling it again. That has some cost in the mutex/spinlock that's behind that
one-time operation, when you're already making the call to set_thread_description conditional.
> Right, maybe it should warn if it failed to set the name?
After you've already printed an error in qemu_thread_naming()? Doesn't seem useful. Or
did you mean in the case we think it should work, but didn't?
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] win32: set threads name
2022-09-30 13:34 ` Richard Henderson
@ 2022-09-30 13:45 ` Marc-André Lureau
2022-09-30 13:55 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Marc-André Lureau @ 2022-09-30 13:45 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, bin.meng, Stefan Weil
[-- Attachment #1: Type: text/plain, Size: 2288 bytes --]
Hi
On Fri, Sep 30, 2022 at 5:35 PM Richard Henderson <
richard.henderson@linaro.org> wrote:
> On 9/30/22 01:08, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Sep 29, 2022 at 9:53 PM Richard Henderson <
> richard.henderson@linaro.org
> > <mailto:richard.henderson@linaro.org>> wrote:
> >
> > On 9/29/22 06:41, marcandre.lureau@redhat.com <mailto:
> marcandre.lureau@redhat.com> wrote:
> > > void qemu_thread_naming(bool enable)
> > > {
> > > /* But note we don't actually name them on Windows yet */
> > > name_threads = enable;
> > >
> > > - fprintf(stderr, "qemu: thread naming not supported on this
> host\n");
> > > + if (enable && !load_set_thread_description()) {
> > > + fprintf(stderr, "qemu: thread naming not supported on
> this host\n");
> > > + }
> > > }
> >
> > Comment is out of date, and I think it would be better to *not* set
> name_threads if not
> > supported, rather than...
> >
> >
> > Comment removed.
> >
> >
> >
> > > +static bool
> > > +set_thread_description(HANDLE h, const char *name)
> > > +{
> > > + HRESULT hr;
> > > + g_autofree wchar_t *namew = NULL;
> > > +
> > > + if (!load_set_thread_description() || !name) {
> > > + return false;
> > > + }
> >
> > ... have to re-query load_set_thread_description later.
> >
> >
> > The load_set_thread_description() function is actually a "one-time"
> function, it doesn't
> > re-load.
>
> You're calling it again. That has some cost in the mutex/spinlock that's
> behind that
> one-time operation, when you're already making the call to
> set_thread_description conditional.
>
That seems pretty marginal given the frequencies of this call, when we are
creating threads.
So you suggest simply setting "name_threads" to false when loading the
function failed?
>
> > Right, maybe it should warn if it failed to set the name?
>
> After you've already printed an error in qemu_thread_naming()? Doesn't
> seem useful. Or
> did you mean in the case we think it should work, but didn't?
>
During qemu_thread_create(), if set_thread_description() failed.
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 3591 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] win32: set threads name
2022-09-30 13:45 ` Marc-André Lureau
@ 2022-09-30 13:55 ` Richard Henderson
0 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2022-09-30 13:55 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel, bin.meng, Stefan Weil
On 9/30/22 06:45, Marc-André Lureau wrote:
> So you suggest simply setting "name_threads" to false when loading the function failed?
Yes.
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-30 14:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-29 13:41 [PATCH] win32: set threads name marcandre.lureau
2022-09-29 17:47 ` Richard Henderson
2022-09-30 8:08 ` Marc-André Lureau
2022-09-30 13:34 ` Richard Henderson
2022-09-30 13:45 ` Marc-André Lureau
2022-09-30 13:55 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).