TBH I was indeed expecting all kinds of hell raining down when I rebuilt the framework with this change, in the form of mysterious linker errors about virtual thunks and what have you.
Not so, no symbols are missing from libKF5CoreAddons because of this patch, so exactly where is the ABI incompatibility?

In a (big) nutshell: D7995.

KDevelop currently uses a single KDirWatch per open project that is fed with a single addDir call which adds all folders under the project directory (and all files, which it shouldn't).
This is fine on Linux when the Inotify method is used, but can become very costly when the QFSWatch method is used (always, on Mac). The actual import of the project directory is done on a background thread (1 per project), and it would make sense to let it feed individual directories to the KDirWatch at the same time. The KDirWatch signals are still received and handled by the main thread.

Using a KDirWatch per thread would still mean that they can be accessing the shared QFSW instance concurrently.

ABI is broken for any derived class, because the added virtual increases the size of the object. The changed offsets would require all code that uses the derived class to be recompiled.

@rjvbb you are changing the memory layout of instances of this class. All applications that use this class / library will break without recompiling.

The KDE Frameworks guarantee binary compatibility backwards. See:
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

To get more familiar with this topic, I can also recommend to watch:
https://m.youtube.com/watch?v=PHrXGHDd9no

In short: there are very strict rules of what you can do with public (=exported, =visible) C++ classes. And adding virtual functions is on the blacklist :)

Using a KDirWatch per thread would still mean that they can be accessing the shared QFSW instance concurrently.

Are you sure it's shared? d->fsWatcher isn't static.
Could it be that QFSW itself uses a single static object internally? In that case it's QSFW that needs to be made threadsafe.

And now that you mention it, I indeed made KDirWatch threadsafe already, at least as far as I could test it on Linux. E.g. KDirWatch::self() is thread-local.

I did see some symptoms that could only be explained by a memory layout change (and apparently caused by this patch). Shame I didn't notice that before posting (I only tested with recompiled code).

Would it be possible to change the name of this virtualised class and use that to make a derived KDirWatch class that just reexports the methods as non-virtual (say with using) - would that change the memory layout too (possibly in even more subtle ways)?

Anyway, pity. Making the class thread-safe internally will probably come with more overhead because it cannot use knowledge about how it's being used. It does look like KDW could do with an internal overhaul (but maybe what I perceive as an overly complex mess is a result of well-tested optimisation? ;)) Is there a reason a single QFSW instance is shared among all KDirWatch instances, for instance?

Hmmm, I'll double-check. I didn't really go beyond the thought that it seemed to make sense to use a single watcher instance if most of the time you'll be watching items that live on a single disk.

Could it be that QFSW itself uses a single static object internally? In that case it's QSFW that needs to be made threadsafe.

That is certainly a possibility, but "needs"? Not everything in Qt is thread-safe that could be. And even if that is done, wouldn't we want to keep KDW working with Qt versions that will never get the patch?

QFSW doesn't have an API to add a directory recursively, does it? That's the KDW aspect that got me into this can of worms: doing that with even a not-so-big source tree as KDevelop takes a noticeable amount of time, for the GCC source tree it takes around 80 seconds (on a 2.7Ghz i7 Mac). And during that call the calling thread is kept busy, so in KDevelop this simply locks up the UI - for each project to be opened in turn.

The safest and cheapest way I see to avoid this is to add single directories in the thread that also does other things with those directories (building up a project representation, i.e. outside of KDW).

The issues I'm seeing without mutex range from lock-ups to import durations that are increased by varying amounts on Mac; the lock-ups occur in the QFSW FSEvents backend. I haven't yet understood that code (or the FSEvents API) enough, but it's quite possible that there's a single instance of something at that level. On Linux I'm getting occasional double-free crashes. The former happens only when I'm adding lots of items in rapid succession (>=2 threads each importing source trees with hundreds of near-empty directories). I've seen the latter happen for more common source trees.

And now that you mention it, I indeed made KDirWatch threadsafe already, at least as far as I could test it on Linux. E.g. KDirWatch::self() is thread-local.

Ah, so that's not a single KDW instance, but one for each thread. That still doesn't mean that you can hand off the instance to a different thread that will add a lot of entries to it.

My initial idea was also that only the QFSW watcher needed mutex protection but as Milian pointed out, there are other things going on when adding an entry that are probably not thread-safe.

OK, my bad, fsWatcher is a KDirWatchPrivate member, not a global and not a static member either. Apologies for that - but it doesn't change the symptoms I'm seeing.

Actually, this does worsen the issue. KDevelop uses a KDW instance per project, and from what I understand, a single import thread per project (@milian, correct me if I'm wrong here). That would suggest that even using a single KDW per thread wouldn't be safe - because that's what we're using already.