Re: kio slaves + kcrash + drkonqi
On Sun, Sep 16, 2018 at 10:48 AM David Faure wrote: > How about just killing the KDE_DEBUG check *and* the code that prints out a > backtrace? I'm always in favor of less code, so I'll prepare a diff for this. > > - Possibly KCRASH_AUTO_RESTARTED needs setting by something in KIO. > > From what I see, if I make a slave crash on every invocation, KIO > > would try to repeat the action it wanted to do by restarting the salve > > ad infinitum when instead it should abort at some point so as to not > > spam the user with drkonqis. > > I don't see why KIO would try to repeat the action. > The app creates a job, it fails because the kioslave crashed, end of story. > There is no autorestart here. > > Although, I can imagine that if you copy one directory with 1000 files and the > kioslave only crashes in the file copy code, you'd get 1000 drkonqis. > But this seems a bit corner case and is all unrelated to this KF5 porting > issue (the missing call to KCrash::initialize). I had an assert fail in GetFileSystemFreeSpace which caused a drkonqi to repeatedly start, but with what I've learned since then I think that was caused by dolphin repeatedly trying to get the info. This isn't something restart handling would or could prevent. With that in mind this type of issue likely needs solving in drkonqi, i.e. I think the problem is with the presentation of the repeated crashing more than the crashing itself. > > The end result is that we still have the stderr backtrace, which I > > think is very useful. > > Did you test it? Was it really useful? Well, it was more useful than no backtrace at all, but as you say the actual symbol names are useless as far as the salve is concerned. > > In fact, by dropping the KDE_DEBUG condition > > it's always enabled even when the user disables drkonqi using > > KDE_DEBUG=1. > > Either this is unsafe (because of memory allocations) and we shouldn't do it, > or if we think it's safe and useful, then as you say it should be done by > kcrash. Conclusion, no, this code shouldn't be kept at all in SlaveBase... > > More details on whether this is safe: the man page says > > "backtrace() and backtrace_symbols_fd() don't call malloc() explicitly, but > they are part of libgcc, which gets loaded dynamically when first used. > Dynamic loading usually triggers a call to malloc(3). If you need certain > calls to these two functions to not allocate memory (in signal handlers, for > example), you need to make sure libgcc is loaded beforehand." > > I'm pretty sure we don't do that... I've had a look and indeed I can not find any explicit call to make sure libgcc is loaded. > > (In fact, I'd even argue that maybe kcrash itself should do the > > backtrace() print. It seems useful in all crash scenarios I'd imagine, > > in particular consider drkonqi can be disabled or not installed. But > > then I don't know if/what performance penalty it'd entail with regards > > to auto restarts.) > > My opinion is that a debugger is much better suited for getting useful > backtraces (including parameter values, and the ability to inspect local > variables). Yes I agree. So, the new plan is to remove the handler code from SlaveBase and instead initialize KCrash. backtrace() code goes away entirely. If someone wants the backtrace() feature it should be moved to KCrash and before setting the crash handler libgcc needs to be explicitly lazy-loaded with a call to any libgcc function. I would suggest people check out coredumpd or another core handler though. HS
Re: kio slaves + kcrash + drkonqi
Hi Harald, On mardi 11 septembre 2018 14:25:15 CEST Harald Sitter wrote: > The debugging faq [1] suggest that this should behave like KCrash. by > default all crashes get drkonqi'd unless you set KDE_DEBUG at which > point drkonqi is disabled. However, having quickly jumped through the > kdelibs history I am not sure that was true even before the kf5 shake > up... Well, it used to be that *just* linking to KCrash would enable it. The need to call initialize() is new in KF5. So when KDE_DEBUG wasn't set, I'm pretty sure you would get drkonqi, and when it was set, the point was to disable drkonqi anyway. > More importantly, why would one bypass kcrash at all? I personally disable drkonqi globally and look for core dumps instead, so I can use gdb to e.g. print local variables. > - SlaveBase calls KCrash::initialize() Yes. That's what's missing. > - SlaveBase drops the KDE_DEBUG check If you want, I suppose KCrash itself will check for it anyway. > - SlaveBase uses KCrash::setCrashHandler to set its own crash handler > to print the backtrace on stderr I'm not sure about this part. 1) that code itself might crash, especially if it has to allocate memory [more on that below] 2) such backtraces are often useless because they only contain exported symbols. Due to hidden visibility being enabled by default, everything else is "???" (the glibc backtrace functionality isn't as clever as gdb...). When this code was written, we didn't have hidden visibility, so it was more useful. How about just killing the KDE_DEBUG check *and* the code that prints out a backtrace? > - Possibly KCRASH_AUTO_RESTARTED needs setting by something in KIO. > From what I see, if I make a slave crash on every invocation, KIO > would try to repeat the action it wanted to do by restarting the salve > ad infinitum when instead it should abort at some point so as to not > spam the user with drkonqis. I don't see why KIO would try to repeat the action. The app creates a job, it fails because the kioslave crashed, end of story. There is no autorestart here. Although, I can imagine that if you copy one directory with 1000 files and the kioslave only crashes in the file copy code, you'd get 1000 drkonqis. But this seems a bit corner case and is all unrelated to this KF5 porting issue (the missing call to KCrash::initialize). > The end result is that we still have the stderr backtrace, which I > think is very useful. Did you test it? Was it really useful? > In fact, by dropping the KDE_DEBUG condition > it's always enabled even when the user disables drkonqi using > KDE_DEBUG=1. Either this is unsafe (because of memory allocations) and we shouldn't do it, or if we think it's safe and useful, then as you say it should be done by kcrash. Conclusion, no, this code shouldn't be kept at all in SlaveBase... More details on whether this is safe: the man page says "backtrace() and backtrace_symbols_fd() don't call malloc() explicitly, but they are part of libgcc, which gets loaded dynamically when first used. Dynamic loading usually triggers a call to malloc(3). If you need certain calls to these two functions to not allocate memory (in signal handlers, for example), you need to make sure libgcc is loaded beforehand." I'm pretty sure we don't do that... > (In fact, I'd even argue that maybe kcrash itself should do the > backtrace() print. It seems useful in all crash scenarios I'd imagine, > in particular consider drkonqi can be disabled or not installed. But > then I don't know if/what performance penalty it'd entail with regards > to auto restarts.) My opinion is that a debugger is much better suited for getting useful backtraces (including parameter values, and the ability to inspect local variables). -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5
kio slaves + kcrash + drkonqi
Hey
I am a bit confused about crash handling in kio slaves. As in: it's
not working the way it should be working and I don't know why it is
the way it is.
The debugging faq [1] suggest that this should behave like KCrash. by
default all crashes get drkonqi'd unless you set KDE_DEBUG at which
point drkonqi is disabled. However, having quickly jumped through the
kdelibs history I am not sure that was true even before the kf5 shake
up...
kio/src/core/slavebase.cpp has
```
if (qEnvironmentVariableIsEmpty("KDE_DEBUG")) {
::signal(SIGSEGV, _handler);
```
sigsegv_handler is a local crash handler that is bypassing kcrash AND
drkonqi entirely and instead prints a backtrace() to stderr.
i.e. by default a slave crash is logged via the local-to-slave crash
handler to stderr. when setting KDE_DEBUG even that logging gets
disabled.
Now to me that makes no sense at all. Why would one want to disable a
simple backtrace print? More importantly, why would one bypass kcrash
at all?
You might rightfully ask why I even care though. The problems are:
a) kcrash isn't used or initialized -> no drkonqi -> no user visible
crash reports about bugged out slaves (as a result it's near useless
to have any q_asserts or qfatals whatsoever in slaves) and we wouldn't
know if the slave quality is garbage
b) since kcrash isn't used -> no recording in dump handler
applications like coredumpd/apport/ABRT -> no way to retrace/report
after the fact either for sysadmins or distros
c) even if kcrash was used it'd still be overridden by the
local-to-slave handler, meaning a slave author can't even force kcrash
use unless they know about this and know how to force the correct
handler to be installed after the local-to-slave handler.
To me it seems all of this is weird and horrible and what I think
would make much more sense is:
- SlaveBase drops the KDE_DEBUG check
- SlaveBase calls KCrash::initialize()
- SlaveBase uses KCrash::setCrashHandler to set its own crash handler
to print the backtrace on stderr
- SlaveBase's backtrace handler then forwards to
KCrash::defaultCrashHandler so shared logic runs (i.e. drkonqi and
then forwarding to dump handlers on linux)
- Possibly KCRASH_AUTO_RESTARTED needs setting by something in KIO.
>From what I see, if I make a slave crash on every invocation, KIO
would try to repeat the action it wanted to do by restarting the salve
ad infinitum when instead it should abort at some point so as to not
spam the user with drkonqis.
The end result is that we still have the stderr backtrace, which I
think is very useful. In fact, by dropping the KDE_DEBUG condition
it's always enabled even when the user disables drkonqi using
KDE_DEBUG=1. The user will get a GUI in the form of drkonqi enabling
them to file a crash report with us so we actually know that our
slaves are crashing. Through KCrash the crash is also forwarded to
system level crash handlers, if applicable, so sysadmins or distros
also know that our slaves are crashing.
(In fact, I'd even argue that maybe kcrash itself should do the
backtrace() print. It seems useful in all crash scenarios I'd imagine,
in particular consider drkonqi can be disabled or not installed. But
then I don't know if/what performance penalty it'd entail with regards
to auto restarts.)
Thoughts?
[1] https://community.kde.org/KDE/FAQs/Debugging_FAQ
