Page MenuHomePhabricator

Fixed https://bugs.llvm.org/show_bug.cgi?id=41584
ClosedPublic

Authored by AndreyChurbanov on May 15 2019, 6:54 AM.

Details

Summary

Removed unconditional unsafe counter decrement at shutdown
that may cause it to become negative if a thread was not active_in_pool
(e.g. is sleeping, or already exited). Same action is done safely on the next line -
inside the call to __kmp_reap_thread.

Diff Detail

Repository
rOMP OpenMP

Event Timeline

tlwilmar accepted this revision.May 15 2019, 7:20 AM

LGTM!

This revision is now accepted and ready to land.May 15 2019, 7:20 AM

For me, this appears to fix my bug report's second reproducer, the one without the num_teams(3). Thanks!

It does not fix the first reproducer, but I applied on an old commit, r359012. I've pulled and will try again later today after the build finishes.

Simply removing a decrement seems very odd. Was it unnecessary in the first place, so did we decrement twice before?

Thanks, Joel.

I actually was not able to reproduce your first problem. But I hope the patch should fix both.

Johannes,

Your guess is true, - it was decremented twice, and with race condition. After "synchronized" decrement there are debug assertion that it is still non-negative. So, if all "broken" decrements happen after the synchronous ones, everything looked fine, otherwise the debug assertion triggered.

Closed by commit rOMP360784: Fixed https://bugs.llvm.org/show_bug.cgi?id=41584. (authored by achurbanov, committed by ). · Explain WhyMay 15 2019, 9:51 AM
This revision was automatically updated to reflect the committed changes.

Thanks, Joel.

I actually was not able to reproduce your first problem. But I hope the patch should fix both.

I still see the assert fail from the first reproducer after applying your patch to r360778, which is from today. This patch seems good, so I'll continue that discussion in bugzilla....

Johannes,

Your guess is true, - it was decremented twice, and with race condition. After "synchronized" decrement there are debug assertion that it is still non-negative. So, if all "broken" decrements happen after the synchronous ones, everything looked fine, otherwise the debug assertion triggered.

Thx!