Don't hit thread-local-storage when entering catch_unwind

[alexcrichton]

Although TLS is fast, it's not that fast on Windows and we shouldn't be hitting it on the fast path for catch_unwind. Right now all we do this for is to reset the panic counter back to 0.

The purpose of this is to allow code like this to work, but thinking more about that I believe that's actually undefined behavior on MSVC. Basically once a panic is initiated you can't initiate another panic for... "reasons". (I'm not 100% clear myself). This definitely seems like a sketchy pattern as well!

So if we don't want to enable that pattern, I think we can get away with different management of the panic counter:

• When a panic starts, bump the panic counter. If this indicates a double panic, abort.
• When a catch_unwind returns from catching a panic, decrease the panic counter. (maybe assert it's 0?)

This way the normal usage of catch_unwind where nothing panics should never hit TLS, and we should still be able to use panics as we do today.

[alexcrichton]

Although not an easy bug to dive into, this shouldn't be too hard to do so. I'd totally be willing to mentor anyone who'd like to tackle this!

[nikhilshagri]

I would like to work on this! I still have a few days before my college starts, so I sure can spend some time working on a not-so-easy bug! 😀

[alexcrichton]

@cynicaldevil go for it! If you need any help let me know, and to clarify the TLS access I'm thinking about is this one right here. I believe we only need to hit TLS once we catch an actual panic, not when we enter or if we leave without catching a panic.

[nikhilshagri]

@alexcrichton Since a panic cannot be started when one's already active, the panic counter can only have two values(0 and 1). Therefore, wouldn't it be better to keep track of this using a bool?

[alexcrichton]

Nah I think we'll need to keep a counter because on a double panic I believe we still invoke the panic hook, and if that panics then the counter will be at 2. The value of 2 is understood as "don't even run the panic hook, just get out of here ASAP"

[nikhilshagri]

@alexcrichton Oh, alright then!

[nikhilshagri]

@alexcrichton Ok, here's what I got after scanning the source code:

• We only need to hit TLS if there's a panic, so the code for checking whether a panic has occurred (this code here) can be moved out of the closure.
• If, and only if we get a panic, we access TLS and decrement the PANIC_COUNT by one, and then return the Err.

Is this correct?

[nagisa]

Basically once a panic is initiated you can't initiate another panic for... "reasons". (I'm not 100% clear myself). This definitely seems like a sketchy pattern as well!

That’s a well known pattern called rethrowing.

If, and only if we get a panic, we access TLS and decrement the PANIC_COUNT by one, and then return the Err.

You also should check that PANIC_COUNT is 0 after decrement in this case, as otherwise you’re either double-panicking or there’s a bug (underflow). While, I believe, the panic!() should handle the double-panic just fine even inside the catch_panic, I feel like it will not properly handle a bug where panicking did not increment PANIC_COUNT for some reason.

[alexcrichton]

@cynicaldevil

Yes I believe that's correct. I think it should be ok to add this at the end of the function (for both the success and happy paths):

debug_assert!(PANIC_COUNT.with(|c| c.get()), 0);

@nagisa

That’s a well known pattern called rethrowing.

Unfortunately this is actually entirely different in how you'd be supposed to codegen it. Thinking more on this now, I've been informed that if you're in a cleanup pad then it's undefined behavior to raise another exception on MSVC. I believe on Unix it's fine, however.

I also believe that rethrowing is implemented by actually catching an exception and then calling library functions to rethrow it. In LLVM a catch pad is quite different from a cleanup pad (which we generate everywhere).

[nikhilshagri]

@alexcrichton If we were to add that statement, then TLS would be accessed anyway, regardless of whether there was a panic or not, which is what we are trying to avoid. I think it must only be added for the path in which a panic occurs.

[nagisa]

Yes, that's correct there's no point in checking that in the panic-less
case.

On Jul 15, 2016 13:42, "Nikhil Shagrithaya" notifications@github.com
wrote:

@alexcrichton https://github.com/alexcrichton If we were to add that
statement, then TLS would be accessed anyway, regardless of whether there
was a panic or not, which is what we are trying to avoid. I think it must
only be added for the path in which a panic occurs.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34787 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AApc0kXLSpN-5qLX7V9zMJ5upDGxFM7uks5qV2QggaJpZM4JKpT0
.

[alexcrichton]

@cynicaldevil ah yeah sure in debug mode it'll hit TLS, but it'll get optimized away in release mode and it's a good sanity check in theory (not required though)