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

[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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20181015/8b6c402c/attachment.html>