remove global statics
Needs RevisionPublic

Authored by brauch on Nov 27 2017, 12:35 PM.

Details

Reviewers
flherne
mwolff
Summary

Having looked at https://bugs.kde.org/show_bug.cgi?id=385107 for a while, it seems to me like compilers do really strange things™ when having multiple nontrivially constructed global static variables in a threaded environment. Probably the best solution is to simply remove all the global statics, and use free functions instead. While we're at it, I turned the Helper class into a namespace, it only has static member functions anyways.

All this code needs lots of refactoring and cleanup other than this change, but we have to start somewhere ...

Test Plan

Tests pass. Bug 385107 is untestable because it's super rare.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
brauch created this revision.Nov 27 2017, 12:35 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptNov 27 2017, 12:35 PM
brauch requested review of this revision.Nov 27 2017, 12:35 PM
flherne requested changes to this revision.Nov 27 2017, 1:19 PM

This is definitely nicer; I remember being a bit puzzled by Helper being a class, and the inscrutable static-var problems are a nuisance.

There are some pre-existing thread-safety issues that should probably be fixed here.

duchain/helpers.cpp
80

Is this necessary? The variable is only used in getDataDirs(). Some of the others are similar.

105

Can these be next to the variable-containing-function they protect?

Does it make sense for this to protect both cachedSearchPaths() and cachedCustomIncludes()? They don't seem particularly related.

304

This doesn't look thread-safe (in either the old or modified version); nothing locks the value while it's being written to.

310–311

ditto

326

ditto

368

ditto

396

ditto...

This revision now requires changes to proceed.Nov 27 2017, 1:19 PM
brauch updated this revision to Diff 23047.Nov 27 2017, 7:40 PM

Adress issues. Inline local statics where the wrapper function is not needed.

Static Initializer Order Fiasco, i guess. You can keep Helper class make access through functions.

brauch marked an inline comment as done.Nov 27 2017, 8:33 PM

Static Initializer Order Fiasco, i guess. You can keep Helper class make access through functions.

Yeah, but do you know what actually happens? I still feel like it's a compiler bug.

flherne requested changes to this revision.Nov 27 2017, 11:23 PM

This version deadlocks KDevelop immediately when opening a Python project. Several threads are waiting on cacheMutex.

This revision now requires changes to proceed.Nov 27 2017, 11:23 PM
anthonyfieroni added inline comments.Nov 28 2017, 5:10 AM
duchain/helpers.cpp
377

You gain mutex at line 377 here it's deadlock, QBasicMutex is not recursive.

brauch updated this revision to Diff 23096.Nov 28 2017, 5:21 PM

next try :D

flherne requested changes to this revision.Nov 28 2017, 10:36 PM
flherne added inline comments.
duchain/helpers.cpp
370

I don't believe this is truly safe. It prevents multiple threads writing simultaneously, but other threads may read while the write takes place, which isn't permitted.

Either the mutex should cover the whole thing, or it might be more performant to use a QReadWriteLock?

Of course, since there was no locking at all before and nothing blew up, this is probably a statistical irrelevance...

397

As above.

This revision now requires changes to proceed.Nov 28 2017, 10:36 PM
brauch updated this revision to Diff 23110.Nov 28 2017, 10:58 PM

Hmpf, right, QList compares its head to its tail which is certainly not atomic. I *think* for QVector this would be safe (it compares its size to zero), but better not take chances. Sorry that this is such a hassle.

mwolff requested changes to this revision.Nov 30 2017, 9:50 AM
mwolff added a subscriber: mwolff.

better, but not fully correct yet.

And yes, global statics are a PITA and should be avoided like hell. One easy way to achieve that is to use lazy initialization in a function-local static, since that is guaranteed to be thread safe by the compiler

duchain/helpers.cpp
318

why is this locking the cache mutex? shouldn't / couldn't you simply lock the duchain here instead?

368

instead of using the mutex for this, initialize this static directly (that is guaranteed to be threadsafe with c++11):

static const  auto correctionFileDirs = QStandardPaths::locateAll(...);
396

dito

482

this is still not thread safe due to calls like this. you should probably ensure this is only being called from the main thread. once you have done that, afaik you can remove the cacheMutex as it won't be needed anymore

This revision now requires changes to proceed.Nov 30 2017, 9:50 AM