Implement named threads on Windows #41736

jsheard · 2017-05-03T23:18:39Z

Adds support for named threads on Windows, using the very awkward convention supported by most debuggers.

rust-highfive · 2017-05-03T23:18:52Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

jsheard · 2017-05-04T03:02:57Z

Hang on, this is broken on 32-bit. I'll fix it tomorrow.

retep998 · 2017-05-04T03:53:35Z

The stack overflow handler is not always guaranteed to be registered, for example cdylibs that are loaded and invoked from other languages. It would perhaps be better to have a separate handler for this that is lazily registered when you set the name.

jsheard · 2017-05-04T15:49:48Z

Good point @retep998, I've moved it to a separate handler.

32-bit is still broken, I think the THREADNAME_INFO struct isn't being packed correctly. The official definition declares it with #pragma pack(8) but as far as I can tell there's no equivalent modifier in Rust. Sorry if I'm missing something obvious.

alexcrichton · 2017-05-04T16:29:40Z

Perhaps the 32-bit definition could manually fill in padding/packing/etc?

retep998 · 2017-05-04T18:34:11Z

I just tested the struct from the example using VC++ and as far as I can tell the #pragma pack(push,8) has no effect on the size or alignment or layout of the struct, neither on 32bit nor 64bit.

retep998 · 2017-05-04T18:36:48Z

src/libstd/sys/windows/thread.rs

+                    mem::size_of::<c::ULONG_PTR>()) as c::DWORD;
+
+        unsafe {
+            c::RaiseException(c::MS_VC_EXCEPTION, 0, size, mem::transmute(&info));


Please don't use transmute. You can safely cast raw pointers &info as *const _ as *const _.

jsheard · 2017-05-04T20:05:57Z

32-bit is now working 🎉

The struct packing was actually fine, the problem was caused by a pre-existing bug. ULONG_PTR is supposed to be a pointer-sized unsigned integer but c.rs currently defines it as an alias for c_ulonglong/u64, so set_name was getting an invalid result from size_of::<ULONG_PTR>() in 32-bit builds.

alexcrichton · 2017-05-04T21:48:59Z

Whoa nice catch @jsheard! It looks like there's a few other locations in the codebase that define ULONG_PTR, mind updating those as well?

alexcrichton · 2017-05-05T03:44:20Z

Awesome thanks @jsheard and thanks with the implementation! Want to squash commits and I'll r+?

jsheard · 2017-05-05T19:16:45Z

I just realized there's a serious bug in my original approach. The magic 0x406D1388 exception code is not reserved for passing messages to the debugger, so if some linked non-Rust code happens to use 0x406D1388 for some other purpose the global filter would silently eat their exceptions.

With 2^32 possible exception codes this is unlikely to happen but that's no excuse 😓

I've pushed a different implementation that should be more robust. Does this look sensible?

retep998 · 2017-05-05T19:42:30Z

It looks like it now ignores all exceptions instead of specifically that exception code?

If only Rust exposed a way to catch SEH exceptions the way you can in Microsoft C...

jsheard · 2017-05-05T20:06:46Z

It does ignore all exceptions if IGNORE_EXCEPTIONS is true, but that is only the case when we are in the process of setting a thread name. So we don't strictly need to check the code explicitly.

That said it might be more efficient to check the code anyway and early-return if it isn't MS_VC_EXCEPTION, to avoid touching the thread-local storage in the common case. Is Rust TLS expensive enough to warrant complicating the exception handler?

alexcrichton · 2017-05-05T22:45:46Z

From the documentation vectored handlers:

The handler should not call functions that acquire synchronization objects or allocate memory, because this can cause problems. Typically, the handler will simply access the exception record and return.

Accessing TLS, however, will on Windows possibly allocate memory and acquire synchronization objects, so using TLS here may not play out so well :(

jsheard · 2017-05-05T22:52:54Z

Argh, I'm out of ideas.

Perhaps we should just abandon the magic exception and focus on SetThreadDescription. It has the downside of only working on Windows 10 and literally no tools actually support it yet (not even Visual Studio), but time will solve that problem eventually.

alexcrichton · 2017-05-05T23:56:06Z

I'd personally be ok with the original implementation, it seems like another excepting colliding with this one is exceedingly rare because it'd break anyone attempting to set the name of a thread like this, right?

jsheard · 2017-05-06T00:58:45Z

@alexcrichton most programs wouldn't set a thread name like this, in C or C++ you would use Microsoft's SEH language extension to catch the exception in a scoped block like this:

__try {
    RaiseException(MS_VC_EXCEPTION, 0, sizeof(info) / sizeof(ULONG_PTR), (ULONG_PTR*)&info);  
} __except (EXCEPTION_CONTINUE_EXECUTION) {}

This is easy and safe, but impossible to express in pure Rust so we're in the unique position of trying to use program-wide VEH callbacks without causing any side effects in the rest of the program.

I'm tempted to give up and just implement SetThreadDescription, it turns out that Windows Performance Analyzer supports it now so we have something to test with. Still waiting for support in MSVC though.

alexcrichton · 2017-05-06T05:02:08Z

If you're feeling intrepid you could actually implement that in Rust! I could help you out poking around in the compiler if you'd like. This'd basically just surface itself as a new intrinsic that's MSVC specific and also specific to the standard library. I'm not 100% sure how much work it would take, though. I believe LLVM is capable of producing the code to match the semantics of that block, so we'd just need to harness it.

jsheard · 2017-05-06T13:34:05Z

I'm sure it's possible, but it would be an insane amount of work and complexity just to implement this one trivial feature. IMO there's no practical and 100% safe way to implement this via the exception.

I'll return to this when debuggers start using the new Set/GetThreadDescription APIs.

alexcrichton · 2017-05-07T16:32:59Z

Ok that's also fine by me, thanks regardless for the PR @jsheard!

nikhilm · 2018-07-18T19:28:06Z

@alexcrichton @jsheard any interest in reviving this by using SetThreadDescription now that Win10 is much more common? I could try to do this if @jsheard is busy. It will need to use GetProcAddress to check if SetThreadDescription is available.

jsheard · 2018-07-18T19:30:07Z

Already did! It's been merged for a while now: #44374

nikhilm · 2018-07-18T19:37:55Z

Sweet! Thank you!

…

rust-highfive assigned alexcrichton May 3, 2017

alexcrichton added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 4, 2017

retep998 reviewed May 4, 2017

View reviewed changes

jsheard added 2 commits May 5, 2017 15:20

Implement named threads on Windows

ceaede8

Use thread-local flag to suppress exceptions

1d5d5cd

jsheard closed this May 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement named threads on Windows #41736

Implement named threads on Windows #41736

jsheard commented May 3, 2017

rust-highfive commented May 3, 2017

jsheard commented May 4, 2017

retep998 commented May 4, 2017

jsheard commented May 4, 2017 •

edited

Loading

alexcrichton commented May 4, 2017

retep998 commented May 4, 2017 •

edited

Loading

retep998 May 4, 2017

jsheard commented May 4, 2017

alexcrichton commented May 4, 2017

alexcrichton commented May 5, 2017

jsheard commented May 5, 2017 •

edited

Loading

retep998 commented May 5, 2017 •

edited

Loading

jsheard commented May 5, 2017 •

edited

Loading

alexcrichton commented May 5, 2017

jsheard commented May 5, 2017

alexcrichton commented May 5, 2017

jsheard commented May 6, 2017 •

edited

Loading

alexcrichton commented May 6, 2017

jsheard commented May 6, 2017

alexcrichton commented May 7, 2017

nikhilm commented Jul 18, 2018

jsheard commented Jul 18, 2018

nikhilm commented Jul 18, 2018 via email

Implement named threads on Windows #41736

Implement named threads on Windows #41736

Conversation

jsheard commented May 3, 2017

rust-highfive commented May 3, 2017

jsheard commented May 4, 2017

retep998 commented May 4, 2017

jsheard commented May 4, 2017 • edited Loading

alexcrichton commented May 4, 2017

retep998 commented May 4, 2017 • edited Loading

retep998 May 4, 2017

Choose a reason for hiding this comment

jsheard commented May 4, 2017

alexcrichton commented May 4, 2017

alexcrichton commented May 5, 2017

jsheard commented May 5, 2017 • edited Loading

retep998 commented May 5, 2017 • edited Loading

jsheard commented May 5, 2017 • edited Loading

alexcrichton commented May 5, 2017

jsheard commented May 5, 2017

alexcrichton commented May 5, 2017

jsheard commented May 6, 2017 • edited Loading

alexcrichton commented May 6, 2017

jsheard commented May 6, 2017

alexcrichton commented May 7, 2017

nikhilm commented Jul 18, 2018

jsheard commented Jul 18, 2018

nikhilm commented Jul 18, 2018 via email

jsheard commented May 4, 2017 •

edited

Loading

retep998 commented May 4, 2017 •

edited

Loading

jsheard commented May 5, 2017 •

edited

Loading

retep998 commented May 5, 2017 •

edited

Loading

jsheard commented May 5, 2017 •

edited

Loading

jsheard commented May 6, 2017 •

edited

Loading