This is an archive of the discontinued LLVM Phabricator instance.

Fix lock order inversion between ManagedStatic and Statistic
ClosedPublic

Authored by inglorion on Apr 6 2018, 6:16 PM.

Details

Summary

Statistic and ManagedStatic both use mutexes. There was a lock order
inversion where, during initialization, Statistic's mutex would be
held while taking ManagedStatic's, and in llvm_shutdown,
ManagedStatic's mutex would be held while taking Statistic's
mutex. This change causes Statistic's initialization code to avoid
holding its mutex while calling ManagedStatic's methods, avoiding the
inversion.

Event Timeline

inglorion created this revision.Apr 6 2018, 6:16 PM
dsanders added inline comments.Apr 6 2018, 6:47 PM
llvm/lib/Support/Statistic.cpp
97

Are you sure llvm_shutdown() calls Statistic::RegisterStatistic()? It's surprising that it would be constructing and registering new Statistic objects in a function that's intended to destroy them.

inglorion updated this revision to Diff 141696.Apr 9 2018, 11:36 AM

Thanks for looking, @dsanders. Does this comment make things clearer?

inglorion updated this revision to Diff 141697.Apr 9 2018, 11:37 AM

slight reword of comment

inglorion marked an inline comment as done.Apr 12 2018, 11:10 AM
inglorion added inline comments.
llvm/lib/Support/Statistic.cpp
97

Ah, not RegisterStatic, but PrintStatistics. I had intended "us" to mean "functions in this part of the code", but seeing that that wasn't clear, I've changed the comment.

dsanders accepted this revision.Apr 16 2018, 9:22 AM

Sorry for the delay, I was out of the office for a few days.

I see how the inversion comes about now and this makes sense to me. LGTM.

Thanks

This revision is now accepted and ready to land.Apr 16 2018, 9:22 AM
This revision was automatically updated to reflect the committed changes.
inglorion marked an inline comment as done.