Refactored code to access TLS only in case of panic (II) #34866
Conversation
|
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. |
nikhilshagri
changed the title
|
No problem! |
…richton Refactored code to access TLS only in case of panic (II) Fixes rust-lang#34787 r? @alexcrichton Do it **very** carefully this time!
|
⌛ Testing commit 0ab8e0e with merge 921eadd... |
|
|
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
|
This might be occurring because when this panic is thrown, another panic already exists. |
|
Hm although in that case wouldn't |
|
Updated PR as per discussion on IRC. |
|
@bors: r+ b36ae59a1b5d5ea3e3dd9b3e48c1c40fc6f002e8 Thansk! |
|
⌛ Testing commit b36ae59 with merge 777ed5a... |
|
💔 Test failed - auto-linux-64-cargotest |
|
@bors: retry On Wed, Jul 20, 2016 at 5:47 PM, bors notifications@github.com wrote:
|
|
⌛ Testing commit b36ae59 with merge 886f1d4... |
|
💔 Test failed - auto-win-msvc-64-opt-mir |
|
@bors: retry sorry for the number of retries... On Thu, Jul 21, 2016 at 12:26 PM, bors notifications@github.com wrote:
|
|
⌛ Testing commit b36ae59 with merge 3df4e9a... |
|
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
|
Hm last few bits of the message -- Looks suspicious, but also perhaps #33434, so I'm gonna retry and see what happens. If it passes on Travis it should pass on bors for a patch like this I think... @bors: retry |
|
@alexcrichton I looked at the source code where this error occurred, and I think two panics are thrown there,which was ok before this patch. but now that we can't throw two panics at once, this runtime error might be occurring. |
|
Oh really? This is the panic, right? I couldn't find a second... |
|
Interesting! Did you configure your local checkout with I think that also means that my hunch from before was correct. When this test panics, it panic with the standard library's machinery that's being tested. The actual panic, however, is then caught with another standard library. The standard library catching the panic did not have its This somehow means that we need to deduplicate the panicking mechanisms here. I think the easiest thing to do would be:
That means that panics during testing should panic through the original standard library. Again though the easiest thing to do is to just move this test out of the standard library, which should allow this PR to land. |
Yes, @eddyb recommended me to add them. They turned out to be useful after all!
This would be a bad idea imo, because there might be other tests where this might be happening. The other solution which you suggested (adding attributes) looks cleaner. |
|
Ok, wanna give that a spin and see if it works? Also this reminds me why it was failing on the bots, they all enable debug assertions! |
|
Well, turns out adding those attributes also brings in a ton of new errors while testing 😅. Think I will move the test for now and see what happens. |
|
How should I move the code inside |
|
Yeah just throwing it into a |
|
Like I suspected, other tests are unable to initiate the panic too. Should I move all such tests into |
|
Hm what sort of magnitude of tests is it turning out to be? If it's a good number them it may be good to rethink the strategy here. This does mean that a failing test in libstd will perhaps have a very obscure error message, which is kinda unfortunate. Could you gist the errors you were getting from the |
|
The process aborts after the first failed panic, so there's no way to determine the number. I will post the logs tomorrow, gotta sleep now :) |
|
grr I had a feeling it was going to amount to something like that. Ok I think that route may be a dead end. I also believe that this works today because the only global state for panics is the Another though, let's try:
if cfg!(test) {
::realstd::rt::bump_panic_counter();
} else {
bump_panic_counter();
}I think all other modifications of the panic counter in theory also need to go through this kind of shim, but for now we can make just the increments go that route. Does that make sense? |
|
Should I do this wherever the panic counter is modified? |
|
I think we can get away with just increments, not reads and decrements, but yeah otherwise wherever the panic counter is incremented we'll need logic like that instead. |
|
I tried to using the function for increments, then for increments and decrements, and neither seem to work.:worried: |
|
Hm ok, I tested out this patch and was able to get |
|
It passes!! 😱 🎉 |
|
@cynicaldevil that's because |
|
@eddyb oh ok. |
nikhilshagri
force-pushed
the
panic-counter
branch
from
b36ae59 to
ea2216c
Compare
Refactored code to access TLS only in case of panic (II) Fixes #34787 r? @alexcrichton Do it **very** carefully this time!
Fixes #34787
r? @alexcrichton
Do it very carefully this time!