D16218: [KDevelop/Core]: safe signal-handler implementation (WIP)

[René J.V. Bertin]

rjvbb created this revision.
rjvbb added a reviewer: KDevelop.
rjvbb added a project: KDevelop.
rjvbb requested review of this revision.

REVISION SUMMARY
This is a work-in-progress change inspired by BKO ticket #399473

Goals:
1- make signal handling safe, i.e. make only async-signal safe calls in the actual signal handler, like `write()` or `sem_post()`.
2- be as cross-platform as possible. ANSI/C signals do exist on MS Windows but I cannot be the judge of whether they'd ever be encountered there.
Bonus goal:

- give users an option to quit a remote session as cleanly as possible if the remote display is inaccessible (whatever the reason).

Goal 1:
Two alternative implementations, both using a static CorePrivate member variable to store the received signal number
a- Use a pair of file descriptors connected via a pipe and a QSocketNotifier. In the signal handler, `write()` to the pipe; this will cause the `CorePrivate::shutdownGracefully` slot to be activated which does the actual work (basically unchanged from the current implementation) outside the signal handler context.
b- Use a semaphore and a monitor started through `QtConcurrent::run`. In the signal handler, `sem_post()` the semaphore which triggers the monitor which then calls `CorePrivate::shutdownGracefully()`.
Goal 2:
Creating a pipe with `pipe()` is Unix-specific; MS Windows has a dedicated call the result of which can (probably) not be fed to QSocketNotifier.
Unnamed semaphores are platform-specific too (note the use of Mach semaphores on Mac which doesn't have unnamed POSIX semaphores) but it should be straightforward to adapt the implementation.

The actual graceful shutdown routine has been modified slightly: it will call `Core::shutdown()` when a 2nd signal is received, before raising that signal again. I propose to make this the only action when a SIGHUP is received, in order to speed up the exit procedure and reduce the chances of getting stuck in the normal exit procedure. This seems more in line with the nature of the signal.

BUG: 399473

TEST PLAN
Launch KDevelop, send it a HUP, INT or TERM signal when it has document(s) with unsaved change open or not. Also test on a remote server.

Tested on Linux, Mac OS and FreeBSD.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

AFFECTED FILES
kdevplatform/shell/CMakeLists.txt
kdevplatform/shell/core.cpp
kdevplatform/shell/core_p.h

To: rjvbb, #kdevelop
Cc: kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb added a comment.


Re: the testing I did on FreeBSD: that's with a dedicated demonstrator; I cannot currently build KDevelop5 there.

INLINE COMMENTS

CMakeLists.txt:162
endif()
+if(UNIX)
+ target_link_libraries(KDevPlatformShell PRIVATE "-pthreads")

I'm not certain why this would be required; linking fails without it (on Linux) when building with Clang. I'm seeing that in 1 or 2 other locations too.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop
Cc: kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

core.cpp:279
+
+#ifdef Q_OS_UNIX
+ if (!s_self) {

kdevelop/kdevplatform/shell/core.cpp:279:2: error: unterminated conditional directive
#ifdef Q_OS_UNIX
^
1 error generated.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop
Cc: arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Gleb Popov]

arrowd added a comment.

Post by René J.V. Bertin
Re: the testing I did on FreeBSD: that's with a dedicated demonstrator; I cannot currently build KDevelop5 there.

I am able to compile KDevelop master with and without your patch on my FreeBSD system. Even without `CMakeLists.txt` change.

What FreeBSD version are you running?

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop
Cc: arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

rjvbb wrote in CMakeLists.txt:162
I'm not certain why this would be required; linking fails without it (on Linux) when building with Clang. I'm seeing that in 1 or 2 other locations too.

The correct way to handle this is `FindThreads` CMake module, I guess.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop
Cc: arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb updated this revision to Diff 43643.
rjvbb added a comment.


Adds the missing #endif

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16218?vs=43640&id=43643

REVISION DETAIL
https://phabricator.kde.org/D16218

AFFECTED FILES
kdevplatform/shell/CMakeLists.txt
kdevplatform/shell/core.cpp
kdevplatform/shell/core_p.h

To: rjvbb, #kdevelop
Cc: arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb marked an inline comment as done.
rjvbb added a comment.

Post by Gleb Popov
I am able to compile KDevelop master with and without your patch on my FreeBSD system. Even without `CMakeLists.txt` change.
What FreeBSD version are you running?

Oh, misunderstanding! I simply don't have a full KF5 build environment on FreeBSD yet. I'm running TrueOS in a VM on a system that isn't exactly powerful, so I'm waiting until "Project Trident" is released with a full KF5 desktop option.

INLINE COMMENTS

Post by Gleb Popov
arrowd wrote in CMakeLists.txt:162
The correct way to handle this is `FindThreads` CMake module, I guess.

But that would probably be a separate change that addresses the issue everywhere, no?

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop
Cc: arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Gleb Popov]

arrowd added a comment.

Post by René J.V. Bertin
Oh, misunderstanding! I simply don't have a full KF5 build environment on FreeBSD yet. I'm running TrueOS in a VM on a system that isn't exactly powerful, so I'm waiting until "Project Trident" is released with a full KF5 desktop option.

It would be much faster for you to get on the track if you download a vanilla FreeBSD VM image and just run

# pkg install kdevelop gmake cmake git
# git clone git://anongit.kde.org/kdevelop.git
# mkdir kdevelop/build
# cd kdevelop/build
# cmake ..
# gmake

INLINE COMMENTS

Post by René J.V. Bertin
rjvbb wrote in CMakeLists.txt:162
But that would probably be a separate change that addresses the issue everywhere, no?

Yep, sure.

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop
Cc: arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Kevin Funk]

kfunk added a comment.


Sorry, but this is yet another unmergeable WIP patch of yours. Just a quick review, I'm not diving into your code (which is difficult to parse, esp. the changes in `CorePrivate::initialize`...) right now.

Questions:

- Why does this patch look so much more verbose than the solution proposed on http://doc.qt.io/qt-5/unix-signals.html?
- Why the alternative implementations? Why do we need to the non-QSocketNotifier variant? You realize that every "alternative solution" or `#ifdef` puts more maintenance burden on us?

The actual graceful shutdown routine has been modified slightly: it will call Core::shutdown() when a 2nd signal is received, before raising that signal again. I propose to make this the only action when a SIGHUP is received, in order to speed up the exit procedure and reduce the chances of getting stuck in the normal exit procedure. This seems more in line with the nature of the signal.

Should be a separate patch, I suppose that's close to a one-line patch against the current code base.

Side note: All the signal handling functionality should be factored out into classes/functions as much as possible in order to simplify the code (cf. http://doc.qt.io/qt-5/unix-signals.html again -- it does that nicely).

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop
Cc: kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Kevin Funk]

kfunk requested changes to this revision.
This revision now requires changes to proceed.

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Sven Brauch]

brauch added a comment.


If there is no solution which does not require two different code paths for handling this problem, I'm against merging this, non-conformant old behaviour or not, sorry.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

Post by Kevin Funk
- Why does this patch look so much more verbose than the solution proposed on http://doc.qt.io/qt-5/unix-signals.html?

Probably because it's a patch against existing code, which needs changes due to the fact that I thought I'd had to avoid static globals.

Post by Kevin Funk
- Why the alternative implementations? Why do we need to the non-QSocketNotifier variant? You realize that every "alternative solution" or `#ifdef` puts more maintenance burden on us?

Who said I wanted to have both committed - the title has `WIP` in it for a reason?! I've explained why there are 2 alternatives (re-read goal 2). There's another reason why avoiding QSocketNotifier could be useful: using a semaphore doesn't require spending 2 file descriptors on something you might never need (it isn't hard to run into the open fd limit with KDevelop, esp. when not running Linux).

Post by Kevin Funk
Should be a separate patch, I suppose that's close to a one-line patch against the current code base.

Which is also why it could be just as well part of a complete overhaul of the whole signal handling stuff, IMHO.

Post by Kevin Funk
Side note: All the signal handling functionality should be factored out into classes/functions as much as possible in order to simplify the code (cf. http://doc.qt.io/qt-5/unix-signals.html again -- it does that nicely).

Which goes against the principle used otherwise to avoid global symbols and stuff everything of that sort into classes...

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

Post by Sven Brauch
If there is no solution which does not require two different code paths for handling this problem, I'm against merging this, non-conformant old behaviour or not, sorry.

So to repeat myself: if signal handling should work on MS Windows (and safely so) then the QSocketNotifier approach is off the table, in principle. I cannot (and do not want to) be judge of whether MS Windows should be supported here.

The semaphore-based implementation should be more direct as it doesn't go over the Qt event loop and signal/slot mechanism. That makes me inclined to prefer it.

I didn't implement a cmake control over `USE_QSOCKETNOTIFIER`, that should have been enough evidence that it wasn't my intention to keep both code paths.

-----

So I should basically convert all static CorePrivate members into global statics?

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Kevin Funk]

kfunk added a comment.

Post by René J.V. Bertin

Post by Kevin Funk
- Why does this patch look so much more verbose than the solution proposed on http://doc.qt.io/qt-5/unix-signals.html?

Probably because it's a patch against existing code, which needs changes due to the fact that I thought I'd had to avoid static globals.

Not sure what you mean by that, but see my last remark in this reply.

Post by René J.V. Bertin

Post by Kevin Funk
- Why the alternative implementations? Why do we need to the non-QSocketNotifier variant? You realize that every "alternative solution" or `#ifdef` puts more maintenance burden on us?

Who said I wanted to have both committed - the title has `WIP` in it for a reason?! I've explained why there are 2 alternatives (re-read goal 2). There's another reason why avoiding QSocketNotifier could be useful: using a semaphore doesn't require spending 2 file descriptors on something you might never need (it isn't hard to run into the open fd limit with KDevelop, esp. when not running Linux).

This KDevelop problem is not solved by saving those 2 of those file descriptors.

Post by René J.V. Bertin

Post by Kevin Funk
Should be a separate patch, I suppose that's close to a one-line patch against the current code base.

Which is also why it could be just as well part of a complete overhaul of the whole signal handling stuff, IMHO.

No, because we now have to discuss several different things as part of one review again, a repeating pattern which makes these discussion so dreadful and exhausting.

1. There's this one behavioral change you wanted, which would fix the bug, a one- or two-line patch, which could be possibly even added to 5.3.
2. Then there's a refactoring patch which should go to master branch, way more invasive and bound to fail for some users on the first iteration.

Post by René J.V. Bertin

Post by Kevin Funk
Side note: All the signal handling functionality should be factored out into classes/functions as much as possible in order to simplify the code (cf. http://doc.qt.io/qt-5/unix-signals.html again -- it does that nicely).

Which goes against the principle used otherwise to avoid global symbols and stuff everything of that sort into classes...

There's no single "global symbol" (and I guess by that you mean "global static"), only class statics. And this implementation is undeniably more easy to read than a scattered dump of blocks of signal-handling related code into an existing class. Not doing so again makes it harder to reason about the code and increases complexity.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Kevin Funk]

kfunk added a comment.

Post by René J.V. Bertin

Post by Sven Brauch
If there is no solution which does not require two different code paths for handling this problem, I'm against merging this, non-conformant old behaviour or not, sorry.

So to repeat myself: if signal handling should work on MS Windows (and safely so) then the QSocketNotifier approach is off the table, in principle. I cannot (and do not want to) be judge of whether MS Windows should be supported here.
The semaphore-based implementation should be more direct as it doesn't go over the Qt event loop and signal/slot mechanism. That makes me inclined to prefer it.

I would recommend to just not think about the Windows requirement, if you cannot test it anyway. We did not have support for signal handling on Windows before; and it's not something commonly needed there either. Either way, *if* we would provide that feature, it'd be done via `SetConsoleCtrlHandler` and catching close events (cf. https://docs.microsoft.com/en-us/windows/console/registering-a-control-handler-function). There existing API for this.

Note: Please don't try to incorporate Windows relevant changes in this review; it would bloat it up even more.

Post by René J.V. Bertin
I didn't implement a cmake control over `USE_QSOCKETNOTIFIER`, that should have been enough evidence that it wasn't my intention to keep both code paths.

Why not keep it *simple* and manageable the *first* iteration to begin with? You're giving yourself a hard time...

Post by René J.V. Bertin
-----
So I should basically convert all static CorePrivate members into global statics?

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

Post by Kevin Funk
I would recommend to just not think about the Windows requirement, if you cannot test it anyway.

You're giving a better reason below, but keeping the possible requirement in mind seems good practice to me. No one would appreciate if someone from another OS universe pushed changes that make implementing a feature harder...

Post by Kevin Funk
We did not have support for signal handling on Windows before

I don't think that's entirely true. The only conditionals I see are the #ifdef SIG* tests, two of which are supposed to succeed on MS Windows because SIGINT and SIGTERM (unless C++ overrides the C standard in this regard).

Post by Kevin Funk
Why not keep it *simple* and manageable the *first* iteration to begin with? You're giving yourself a hard time...

I *could* have presented the semaphore-based implementation only, of course. A semaphore seems just more natural to be used in this kind of context than doing poll() (or select() or whatever QSocketNotifier does behind the scenes) on a file descriptor to detect writes to a pipe.

BTW

Post by Kevin Funk
This KDevelop problem is not solved by saving those 2 of those file
descriptors.

No, but increasing the count from the start is only going to make it (a wee bit) worse - and it can make the difference between crashing (aborting) or not.
Some BSD variants apparently still allow as little as 256 open files, and then 2 is a much less innocent number than when you basically have an unlimited open file count.

But: this whole argument is moot when QtConcurrent::run() comes with a relevant and significant overhead. I haven't found evidence of that but apparently it does, so I'd have to manhandle pthread instead. Not a problem if the cross-platform argument is moot, but it requires more coding.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb updated this revision to Diff 43662.
rjvbb added a comment.


- Dropped the semaphore-based alternative as it would only be less resource-intensive when manhandling pthreads
- moved almost everything back out of the CorePrivate class, keeping only the slot required for QSocketNotifier operation.
- dropped the SIGHUP shortcut, to be resubmitted in the future

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16218?vs=43643&id=43662

REVISION DETAIL
https://phabricator.kde.org/D16218

AFFECTED FILES
kdevplatform/shell/core.cpp
kdevplatform/shell/core_p.h

To: rjvbb, #kdevelop, kfunk
Cc: brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Aaron Puchert]

aaronpuchert added inline comments.

INLINE COMMENTS

core.cpp:58
+static int signalPipeWrite = -1;
+static volatile std::sig_atomic_t signalReceived = 0;
+static QSocketNotifier* signalNotifier = nullptr;

I think it's better to use `std::atomic` here, `volatile` should be a thing of the past.

core.cpp:68-69
if ( !handlingSignal ) {
handlingSignal = 1;
+ qCWarning(SHELL) << "Going down on signal" << sig;

Is this supposed to be thread-safe? Because it isn't. If it should be thread-safe, make `handlingSignal` a `std::atomic` (non-volatile) and use `compare_exchange_strong`.

core.cpp:79-83
+ signalNotifier->setEnabled(false);
+
// re-raise signal with default handler and trigger program termination
std::signal(sig, SIG_DFL);
std::raise(sig);

Shouldn't this be in the `if` as well or what am I missing?

core.cpp:86
+void signalHandler(int sig)
+{

If I read the standard correctly, this needs C linkage, so wrap it in `extern "C" {...}`.

core.cpp:90
+ if (signalPipeWrite != -1) {
+ qCDebug(SHELL) << "signal" << sig << " received, shutting down gracefully";
+ write(signalPipeWrite, &sig, sizeof(sig));

Can you use this in a signal handler?

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb marked an inline comment as done.
rjvbb added inline comments.

INLINE COMMENTS

aaronpuchert wrote in core.cpp:58
I think it's better to use `std::atomic` here, `volatile` should be a thing of the past.

From what I've read std::atomic is safe only if its `is_lock_free` property is true. If not, sig_atomic_t is the type of choice. (You can't lock a mutex in a signal handler.)

As to the volatile property, I just copied that from `handlingSignal`.

aaronpuchert wrote in core.cpp:68-69
Is this supposed to be thread-safe? Because it isn't. If it should be thread-safe, make `handlingSignal` a `std::atomic` (non-volatile) and use `compare_exchange_strong`.

I don't think there's a need for that.

aaronpuchert wrote in core.cpp:79-83
Shouldn't this be in the `if` as well or what am I missing?

No, the shutdown procedure seems to be designed to be a 2-stage process. The first interrupt will trigger a regular shutdown procedure (inside the if) and lead to exit if all goes well. If that process blocks or if another signal comes in before it can terminate, the part outside the if is executed.

In its current form you could indeed put the SIG_DFL statement inside or before the if. In my original proposal there was a small addition here: shutdown of just the core plugin, evidently before restoring the default signal handler. That change will be resubmitted so I'd rather not move statements now that I'd have to move back again.

aaronpuchert wrote in core.cpp:86
If I read the standard correctly, this needs C linkage, so wrap it in `extern "C" {...}`.

Does that do anything other than not mangling function names?

aaronpuchert wrote in core.cpp:90
Can you use this in a signal handler?

there's a good chance it will allocate memory, indeed.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Aaron Puchert]

aaronpuchert added a comment.


You're right, my remarks apply to the existing code already.

In a POSIX world we could just block the signals and poll <https://linux.die.net/man/2/sigwaitinfo> for them in the main loop instead of writing these insane signal handlers, but our standard writers didn't want to leave room for that apparently.

INLINE COMMENTS

rjvbb wrote in core.cpp:58
From what I've read std::atomic is safe only if its `is_lock_free` property is true. If not, sig_atomic_t is the type of choice. (You can't lock a mutex in a signal handler.)
As to the volatile property, I just copied that from `handlingSignal`.

At least `std::atomic<bool>` is lock-free on all relevant architectures. (I'm aware that the standard doesn't guarantee it, probably because of some embedded architectures. With C++17 we could use std::atomic::is_always_lock_free in a static_assert.)

rjvbb wrote in core.cpp:68-69
I don't think there's a need for that.

Either two signals can be handled at the same time, then it needs to be thread-safe, or there is always only one signal handled at the same time, in which case this `if` isn't needed.

rjvbb wrote in core.cpp:79-83
No, the shutdown procedure seems to be designed to be a 2-stage process. The first interrupt will trigger a regular shutdown procedure (inside the if) and lead to exit if all goes well. If that process blocks or if another signal comes in before it can terminate, the part outside the if is executed.
In its current form you could indeed put the SIG_DFL statement inside or before the if. In my original proposal there was a small addition here: shutdown of just the core plugin, evidently before restoring the default signal handler. That change will be resubmitted so I'd rather not move statements now that I'd have to move back again.
The first interrupt will trigger a regular shutdown procedure (inside the if) and lead to exit if all goes well. If that process blocks or if another signal comes in before it can terminate, the part outside the if is executed.

But why? If I want an unclean shutdown, I can just send SIGKILL. I don't understand why a second signal should be handled differently than the first.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb marked an inline comment as done.
rjvbb added a comment.

Post by Aaron Puchert
At least `std::atomic<bool>` is lock-free on all relevant architectures. (I'm aware that the standard doesn't guarantee it, probably because of some embedded architectures.

But is it always lock-free on the relevant architectures? Or rather:

- what determines if the type is lock-free
- what are "the relevant architectures"?

Post by Aaron Puchert

rjvbb wrote in core.cpp:68-69

Either two signals can be handled at the same time, then it needs to be thread-safe

A signal that arrives before the previous has been handled, that would probably be on the same (= main) thread and thus require reentrancy (I always forget if that's more or less strict than thread safety). Atomic variables protect against multiple threads accessing them at the same time AFAIK, but can they do that for concurrent access from the same thread?
Either way, can a signal handler really be called in the middle of an assignment (to an atomic variable)? If not, then only 1 instance will get access to the inside of the if, no?

Post by Aaron Puchert
, or there is always only one signal handled at the same time, in which case this `if` isn't needed.

No, because the conditional is static so it gives access into the if only once.

And there's an obvious situation where a 2nd signal can arrive before we're done: when something (in the UI) blocks and the user repeats the kill command.

Post by Aaron Puchert
But why? If I want an unclean shutdown, I can just send SIGKILL. I don't understand why a second signal should be handled differently than the first.

I guess the idea was to force the exit at the 2nd arrival of one of the caught signals, so you can just use your command history to repeat the same kill command.
I probably wouldn't have implemented this way either unless I wanted to try something else on the 2nd signal (again, as I intend to do here). But it doesn't bother me either; someone thought it was a good idea in the past, and it makes it easy to add a 2nd chance, "bare bones" clean exit attempt upon receipt of a 2nd signal (or on the 1st in case of a HUP).

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Aaron Puchert]

aaronpuchert added a comment.

Post by René J.V. Bertin

Post by Aaron Puchert
At least `std::atomic<bool>` is lock-free on all relevant architectures. (I'm aware that the standard doesn't guarantee it, probably because of some embedded architectures.

- what determines if the type is lock-free
- what are "the relevant architectures"?

This is determined by hardware support. With relevant architectures I meant architectures on which Qt/KDE applications can run, like x86, PPC, ARM. These all have lock-free `std::atomic<bool>`, as far as I know. I guess you can also use `std::atomic_flag`, which is guaranteed to be lock-free, and should offer enough functionality for our use case.

Post by René J.V. Bertin
Either way, can a signal handler really be called in the middle of an assignment (to an atomic variable)? If not, then only 1 instance will get access to the inside of the if, no?

Not in the middle of an assignment, but between the read in `if (!handlingSignal)` and the write `handlingSignal = 1` it can be interrupted. That's not atomic.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

Post by Aaron Puchert
I guess you can also use `std::atomic_flag`, which is guaranteed to be lock-free, and should offer enough functionality for our use case.

...

Post by Aaron Puchert
Not in the middle of an assignment, but between the read in `if (!handlingSignal)` and the write `handlingSignal = 1` it can be interrupted. That's not atomic.

So is it enough to change to std::atomic_flag or should we use some sort of critical section with an actual lock? Because for handlingSignal that's possible as it is outside of the actual signal handler.

Should I use std::atomic_flag for the m_signalReceived member too?

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

This is determined by hardware support. With relevant architectures I meant architectures on which Qt/KDE applications can run, like x86, PPC, ARM. > These all have lock-free `std::atomic<bool>`, as far as I know.

<aside>
I presume that underneath this would use "intrinsics" like _InterlockedIncrement does (https://github.com/RJVB/SynchroTr/blob/master/CritSectEx/msemul.h#L548) so yeah, hardware determined. But the standard depends on software and this could decide to use locks under certain conditions. Even runtime conditions if that same standard doesn't guarantee it won't.
That's requires a pretty intimate knowledge of both the standard and its implementation(s), and I guess that's why the StackOverflow discussions I've seen seem to agree that it's best to avoid std::atomic<?> if it being lock free is a hard requirement.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.


Any idea how we could test the race-condition scenario? I've been trying by re-raising the signal from just before and just inside the if(handlingSignal) loop and until now it has always been behaving as expected (= the if is executed only once).

It seems in fact that the gracefulExit *slot* is not being called at all when a previous call is ongoing; are you sure Qt doesn't prevent that from happening but instead queues signals for sequential delivery? If so your concern seems moot.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Aaron Puchert]

aaronpuchert added a comment.

Post by René J.V. Bertin
So is it enough to change to std::atomic_flag or should we use some sort of critical section with an actual lock? Because for handlingSignal that's possible as it is outside of the actual signal handler.

A lock is not needed. Just change the if to `if (!test_and_set()) { ... }` if you go with `std::atomic_flag`.

By the way, since `shutdownGracefully` is no longer the actual signal handler, it wouldn't be a problem if it weren't lock-free.

Post by René J.V. Bertin
Should I use std::atomic_flag for the m_signalReceived member too?

You can't use std::atomic_flag, because it's an `int`. There is a race condition anyway which you won't get rid of using atomics: another signal could have overwritten `signalReceived` before you read it. The good news is that we don't need the variable at all: since you write `sig` to the pipe, you should be able to read it at the other end, and you should probably get multiple signals in the right order.

Post by René J.V. Bertin
It seems in fact that the gracefulExit *slot* is not being called at all when a previous call is ongoing; are you sure Qt doesn't prevent that from happening but instead queues signals for sequential delivery? If so your concern seems moot.

It's possible that either the OS decides not to interrupt the signal handler or that Qt serializes (Qt) signals. I don't know. I was just saying that //if// the `if` is necessary, then it must test and set the flag atomically.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

Post by Aaron Puchert
You can't use std::atomic_flag, because it's an `int`.

Yes, I realised that when I started tinkering with the various std::atomic* types.

Post by Aaron Puchert
There is a race condition anyway which you won't get rid of using atomics: another signal could have overwritten `signalReceived` before you read it.

Indeed. I did use the value written to the pipe at first, but that feels a bit awkward and I don't know to what extent we can be certain that the read won't block and/or will always read the actual (entire) value.
Either way, I decided that this race condition is not an issue:

- signalReceived can only take values that correspond to the signals we handle
- currently all signals trigger the same series of reactions
- if 2 signals arrive so quickly one after another that the socketnotifier didn't have time to react it doesn't seem unreasonable to let the later signal override the earlier signal.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.


Still working on this and waiting on more background info from the Qt Interest ML.

Somehow I missed that the 2-stage design cannot work (or it stopped working for some reason) when proxying interrupt signals over a QSocketNotifier signal/slot mechanism. I thought I had tested that with my semaphore-based implementation, but this *would* also be another effect of Qt's event loop queueing slot evocations instead of invoking them "recursively".

Indeed, when I use a non-specified connection from the QSocketNotifier signal the second signal never makes it into the graceful exit procedure when I do

kill -2 $PID ; sleep 1 ; kill -1 $PID

The actual signal handler does see all signals, of course, and the 2nd signal is the one seen by the graceful procedure when I do

kill -2 $PID ; kill -1 $PID

FWIW, I also tried reading from the pipe to see if something at that level was blocking the 2nd signal, and if an explicit Qt::DirectConnection made a difference. Not.

So I may yet have to implement a more basic approach: resetting the signal handling (signal(sig, SIG_DFL)) in the actual signal handler.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb updated this revision to Diff 43922.
rjvbb added a comment.


Drops the potentially unsafe qDebug output in the signal handler, makes `handlingSignal` an `std::atomic_bool` (since we can use locks here anyway) and adds copious comments.

Contrary to what I wrote before the 2-stage mechanism does work when the pipe is "flushed", and it turns out this is required to prevent multiple firing from QSocketNotifier (which seems to react to the presence of unread data on the pipe rather than to write operations, at least in Qt 5.9).

This means `signalReceived` is somewhat redudant. I do actually like how this variable makes the mechanism use the last signal from a signal "burst" but am not married to it either.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16218?vs=43662&id=43922

REVISION DETAIL
https://phabricator.kde.org/D16218

AFFECTED FILES
kdevplatform/shell/core.cpp
kdevplatform/shell/core_p.h

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Aaron Puchert]

aaronpuchert added inline comments.

INLINE COMMENTS

core.cpp:66
+ // sense of the term.
+ static std::atomic_bool handlingSignal(false);
+ int sig = signalReceived;

The `atomic_` aliases are provided for compatibility with C code, I'd just use `std::atomic<bool>` directly.

core.cpp:67-69
+ int sig = signalReceived;
+
+ signalReceived = 0;

Why keep `signalReceived` if you can get the same value from the pipe?

core.cpp:79-80
+
+ if (!handlingSignal) {
+ handlingSignal = true;
+ // the first time we see a signal we attempt to exit

Still not atomic.

if (!handlingSignal.exchange(true)) {

core.cpp:89-90
app->quit();
return;
+ } else {
+ qCWarning(SHELL) << "Going down harder on signal" << sig;

No else after return.

core.cpp:106-112
+ // NOTE: using signalReceived is not thread-safe. If a 2nd signal comes in
+ // before the current signal has been processed, shutdownGracefully() will
+ // see the new value. That is not a problem in this case; the result is that
+ // the handler uses the last of a "signal burst", which is not unreasonable
+ // given that this currently makes no difference at all in the graceful
+ // shutdown behaviour.
+ signalReceived = sig;

That might be true, but why bother? You write the signal value to the pipe, so you might as well read it on the other end.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb marked 5 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

aaronpuchert wrote in core.cpp:66
The `atomic_` aliases are provided for compatibility with C code, I'd just use `std::atomic<bool>` directly.

your call, if you think it's more readable.

aaronpuchert wrote in core.cpp:67-69
Why keep `signalReceived` if you can get the same value from the pipe?

As I said, I find it awkward to use an IPC method to get a value that likely doesn't even come from another thread in the same process, and I can't shake the feeling that I might not read back the value I wrote.

Again, your call...

aaronpuchert wrote in core.cpp:79-80
Still not atomic.
if (!handlingSignal.exchange(true)) {

Doh, of course.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb updated this revision to Diff 43962.
rjvbb marked 3 inline comments as done.
rjvbb added a comment.


updated as requested.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16218?vs=43922&id=43962

REVISION DETAIL
https://phabricator.kde.org/D16218

AFFECTED FILES
kdevplatform/shell/core.cpp
kdevplatform/shell/core_p.h

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Aaron Puchert]

aaronpuchert accepted this revision as: aaronpuchert.
aaronpuchert added a comment.


The signal handling and processing gets a +1 from me, but someone else should review the overall code structure.

Post by Kevin Funk
Side note: All the signal handling functionality should be factored out into classes/functions as much as possible in order to simplify the code (cf. http://doc.qt.io/qt-5/unix-signals.html again -- it does that nicely).

Arguably that still stands, but it's outside my area of expertise to comment on it.

INLINE COMMENTS

Post by Kevin Funk
core.cpp:68-69
+ // Get the signal number that was written to the pipe in the actual
+ // signal handler. A bit awkward but we have to flush the read side
+ // of the pipe anyway.
+ // Failure to flush can lead to repetitive signals from QSocketNotifier

I don't find this awkward. You're reacting to a pipe write, so that seems like the right way to get the information that you want to react to.

Post by Kevin Funk
core.cpp:103
+ write(signalPipeWrite, &sig, sizeof(sig));
+ }
+}

Maybe you can add an else branch where you re-raise the signal with the default handler.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk, aaronpuchert
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

Post by Aaron Puchert
Arguably that still stands, but it's outside my area of expertise to comment on it.

FWIW, I took Kevin's remark about this as a reaction to the fact that I moved the signal handling stuff into the CorePrivate class, and a request to take it back out as it was before. I think his other remarks in the exchange we had mean that he didn't intend me to write a dedicated signal handling class. I still could of course, but is it really worth the effort given that there's unlikely to be reuse and the only reason to instantiate the class would be to have a target to connect to?

Post by Aaron Puchert

Post by Kevin Funk
core.cpp:103
+ write(signalPipeWrite, &sig, sizeof(sig));
+ }
+}

Maybe you can add an else branch where you re-raise the signal with the default handler.

If the goal is just to let a 2nd signal trigger the default reaction from the runtime then it can be even simpler. No need for an else, a `signal(sig, SIG_DFL)` after that write() would do the same thing.
But remember there was a 2nd aspect to this patch initially which I've been told to submit in a separate patch: handle SIGHUP with a more basic shutdown method, the same as used in case a second signal is handled.

Of course that could also be done in a different way: connect the QSocketNotifier signal to a different slot ("shutDownSafelyNow()") before attempting the graceful exit (and call that slot when a SIGHUP arrives):

shutDownSafelyNow()
{
std::signal(sig, SIG_DFL);

if (corePrivateInstance->m_core) {
// shutdown core functionality, in particular the DUChain subsystem
// in an effort to prevent cache corruption. It's only cache, but
// regenerating it can be very time-consuming.
corePrivateInstance->m_core->shutdown();
}

signalNotifier->setEnabled(false);

// re-raise signal with default handler and trigger program termination
std::raise(sig);
}

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk, aaronpuchert
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb retitled this revision from "[KDevelop/Core]: safe signal-handler implementation (WIP)" to "[KDevelop/Core]: safe signal-handler implementation".

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk, aaronpuchert
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Aaron Puchert]

aaronpuchert added a comment.

Post by René J.V. Bertin

Post by Aaron Puchert
Maybe you can add an else branch where you re-raise the signal with the default handler.

If the goal is just to let a 2nd signal trigger the default reaction from the runtime then it can be even simpler.

The goal would be to handle the case `signalPipeWrite == -1`. Although I'm actually not sure why we need to check that in the first place, because you register the signal handler only //after// creating the pipe has succeeded. Then `signalPipeWrite != -1` should be a given, but you could add an assertion (when creating the pipe) if you want.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk, aaronpuchert
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb updated this revision to Diff 45458.
rjvbb marked an inline comment as done.
rjvbb added a comment.


Aaron's last suggestion made me realise I forget a few things when porting the patch back to using QSocketNotifier.
The shutdown procedure was intended to (and now does) include closing the signal pipe, verifying the descriptor before writing to it makes sense. So does handling failure there by re-raising the signal with the default handler.
I thought it would be even more robust to move the actual write into the if and check whether it succeeded, after all we want to be certain that these signals are always handled.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16218?vs=43962&id=45458

REVISION DETAIL
https://phabricator.kde.org/D16218

AFFECTED FILES
kdevplatform/shell/core.cpp
kdevplatform/shell/core_p.h

To: rjvbb, #kdevelop, kfunk, aaronpuchert
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk, aaronpuchert
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb marked 2 inline comments as done.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk, aaronpuchert
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Aaron Puchert]

aaronpuchert added inline comments.

INLINE COMMENTS

core.cpp:78-80
+#ifdef SIGHUP
+ && sig != SIGHUP
+#endif

Why that? Can't we take the time for an orderly shutdown on SIGHUP?

core.cpp:333
- installSignalHandler();
+// Open a pipe or an eventfd, then install your signal handler. In that signal

Isn't the indentation a bit weird?

core.cpp:399
+ close(signalPipeRead);
+ }
}

Maybe `signalPipeRead = -1` as well?

core.cpp:438-444
+#ifdef OPEN_MAX
+ // set the maximum number of files
+ struct rlimit rlim;
+ getrlimit(RLIMIT_NOFILE, &rlim);
+ rlim.rlim_cur = qMin(rlim_t(OPEN_MAX), rlim.rlim_max);
+ setrlimit(RLIMIT_NOFILE, &rlim);
+#endif

Looks unrelated to me. What does this have to do with signal handling?

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk, aaronpuchert
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.


Sh@@t, sorry, I allowed some unrelated (and potential future) changes to pollute this version. Will fix tomorrow.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk, aaronpuchert
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight