DrKonqi : lldb and Mac support
ClosedPublic

Authored by rjvbb on Mar 3 2017, 10:26 PM.

Details

Summary

As touched upon previously, DrKonqi is a utility that can be very useful outside of Plasma desktop environments too, and as such it should at the very least have basic support for the lldb debugger. This is not only of interest outside of Plasma; FreeBSD uses clang as its system compiler (AFAIK), and it will thus probably also use lldb as its default debugger.

This patch introduces that support, providing useful backtraces in bug tickets created through DrKonqi. Attaching lldb to the crashed executable is currently done through a wrapper script that invokes Apple's Terminal.app . That could probably be improved in the future, possibly even by attaching Xcode to the application.

I've included a few minor adaptations for use of DrKonqi outside of Plasma DEs (preserve existing app/window icons when QIcon::fromTheme() fails and forcing DrKonqi to the foreground on Mac).

Test Plan

This patch has been tested on Mac for over a year now; I have submitted numerous bug reports with it.

The backtrace parser could probably be improved.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 5417
Build 5435: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application edited projects, added Plasma; removed Plasma: Workspaces. · View Herald TranscriptMar 3 2017, 10:26 PM

Do apps even ship DrKonqi? It's part of plasma-workspace, which isn't meant for non-X11/Wayland, after all.

rjvbb added a comment.EditedMar 3 2017, 10:36 PM

Not currently, but DrKonqi ought *really* be elsewhere. There's nothing Plasma-specific to it. On the contrary, it should be easily available on every platform for which KDE applications are available so that every KDE user can contribute to KDE improvement by creating useful bug reports. This has been discussed before.

If memory serves me well DrKonqi is launched by KCrash and IMHO it would make sense to incorporate it there.
Edit: at the very least it should be in its own project so that it can be built more easily, like kde-cli-tools (another project that's got little if any exclusivity to Plasma).

rjvbb retitled this revision from DrKonqi : lldb support (and cross-platform adaptation) to DrKonqi : lldb support.Mar 6 2017, 9:07 AM
rjvbb edited the summary of this revision. (Show Details)
mart added a subscriber: mart.Mar 6 2017, 1:13 PM

hmm, frameworksintegration? (but still in frameworks side)

rjvbb added a comment.Mar 6 2017, 1:39 PM
In D4929#93093, @mart wrote:

hmm, frameworksintegration? (but still in frameworks side)

Bundling with, you mean? That could work too.

It's not really the topic of this review request, but I discussed the idea of bundling with someone who bundles a well-known KDE app for Mac and MS Windows. I'll leave it to him to come forward (or not) but it looks we both think that KCrash would be the most logical framework.

Many applications already depend on it (not the case with frameworksintegration) so done right no extra action would be required to build and include DrKonqi in binary distribution forms of those applications. More useful bug reports concerning crashers (because a backtrace is attached) for little effort, who would not appreciate that?

KCrash would probably have to become a Tier3 framework - no idea if that's an issue in itself or not.

kfunk requested changes to this revision.Mar 7 2017, 8:45 PM
kfunk added a subscriber: kfunk.

So, plasma-workspace is usually way out of my comfort zone, I'm commenting anyway since RJVB urged me to do it.

See my notes.

drkonqi/backtracegenerator.cpp
62 ↗(On Diff #12148)

Please do this only if lldb is used then. No need to change code which apparently worked fine for years. Also, did you try QProcess::terminate instead?

PS: QProcess gets unhappy when being deleted while still running (=> runtime warnings).

PPS: Please read my other advice about quitting LLDB below, too

144 ↗(On Diff #12148)

This whole logic looks really cumbersome.

Is there really no way to exit LLDB cleanly after detach?

http://stackoverflow.com/questions/26267289/how-can-i-exit-lldb-after-running-commands-with-o suggests there is:

lldb /bin/ls -o "run" -o "script import os; os._exit(1)"

I take it not everyone's got Python on they system, but this works for me as well:

lldb -p $(pidof kate) -o detach -o quit

Just put that into the -o quit in the lldbrc?

drkonqi/backtracegenerator.h
87 ↗(On Diff #12148)

Looks pretty unclean to have a backend-specific variable around here.

drkonqi/data/AppleTerminal
1 ↗(On Diff #12148)

Please explain why this file is needed(?)

drkonqi/data/debuggers/internal/lldbrc
9 ↗(On Diff #12148)

set set? Really?

drkonqi/drkonqibackends.cpp
165 ↗(On Diff #12148)

Why these special conditions? Needs comments.

Whether to use or not to use lldb on a particular OS X version should be runtime decision, too.

drkonqi/parser/backtraceparserlldb.cpp
26 ↗(On Diff #12148)

Style: Use const QString &line

More of these issues in other lines

drkonqi/parser/backtraceparserlldb.h
28 ↗(On Diff #12148)

nullptr

31 ↗(On Diff #12148)

Use override, strip virtual

This revision now requires changes to proceed.Mar 7 2017, 8:45 PM
rjvbb marked 4 inline comments as done.Mar 10 2017, 12:52 PM
rjvbb added inline comments.
drkonqi/backtracegenerator.cpp
62 ↗(On Diff #12148)

It's been a while, QProcess::terminate() was an approach I remember preferring to avoid possibly because it also led to runtime warnings. I also don't think there's much of a difference in what ::kill() and ::terminate() do.
With deleteLater() I rarely if ever see warning messages, probably because it's an implicit way of waiting long enough (in the background) for lldb to exit.

144 ↗(On Diff #12148)

-o quit indeed works with newer lldb versions, but not yet with the system version on OS X 10.9 .

I suppose I could do a combination of both. The logical way would be \nquit\nscript import os; os._exit(0) but processing stops after the quit command so it'd have to be the opposite. I'd have to test this for a bit.

Not having a python interpreter may not be an issue. I don't know how its complete absence is treated but errors in the python expression (loading an inexisting module for instance) don't stop processing of subsequent commands.

Can one put comments in the lldbrc file explaining the reason of the surprising BatchCommands set?

drkonqi/data/AppleTerminal
1 ↗(On Diff #12148)

It is required to attach lldb in a terminal window on Mac using the native terminal application. I don't want to rely on konsole being installed there, in part because that application has issues with certain Control keystrokes under Qt5 (including Ctrl-C and family).

drkonqi/drkonqibackends.cpp
165 ↗(On Diff #12148)

Actually, there can be a runtime decision to use lldb on earlier (10.7-) versions but not on later versions. Gdb doesn't work properly from 10.8 onwards. And to be honest lldb was in such an early stage in 10.7 and earlier that you wouldn't want to use the system version there.
Given that gdb still works there it seems unnecessarily complicated to make this a runtime decision. Esp. since using gdb means you get access to the standard backtrace parser.

I'll put a concise version of this explanation as a comment.

drkonqi/parser/backtraceparserlldb.cpp
26 ↗(On Diff #12148)

I think I preserved the style in the backtracerparser implementation I cloned...

rjvbb marked 8 inline comments as done.Mar 10 2017, 1:37 PM
rjvbb added inline comments.
drkonqi/backtracegenerator.cpp
62 ↗(On Diff #12148)

Re (sic:) KProcess::terminate(): see the hunk around line 145!

rjvbb updated this revision to Diff 12360.Mar 10 2017, 1:57 PM
rjvbb edited edge metadata.

This new version incorporates most of the changes Kevin requested.

Quitting lldb through

  1. a python "lambda"
  2. the quit command

seems to work reliably enough on Mac.

It still uses AppleTerminal.sh unconditionally, I'll need to see how I can install and invoke that script only on Mac.

rjvbb added inline comments.Mar 10 2017, 2:03 PM
drkonqi/backtracegenerator.h
87 ↗(On Diff #12148)

I missed this one. I don't think it's possible to detect detaching (from the debugger output!) elsewhere but in the bt. generator. Not without adding a line parser to the backend.

That *would* be the proper way to proceed if other backends required detecting a detached debugger too, but apparently that isn't the case.

rjvbb added inline comments.Mar 10 2017, 5:49 PM
drkonqi/backtracegenerator.h
87 ↗(On Diff #12148)

I suppose I could replace m_lldbDetached with something like

bool Debugger::isDetached(const QString &outputLine=QString())
{
    if (!outputLine.isEmpty() && codeName() == "lldb") {
        QString line = outputLine.simplified();
        if (/*line contains "Process detached"*/) {
            m_isDetached = true;
        }
    }
    return m_isDetached;
}

and call that function instead of reading/setting m_lldbDetached.

Whether that's really less cumbersome I don't know?

rjvbb updated this revision to Diff 12368.Mar 10 2017, 5:55 PM

platform-specific data/debuggers/external *rc files and installation of the AppleTerminal script.

gdb and lldb are now both started via that script on Mac and via konsole elsewhere.

What is the actual state of this patch ?

This revision now requires changes to proceed.Nov 22 2018, 2:00 PM
rjvbb marked an inline comment as done.Nov 22 2018, 4:42 PM

Sorry, I had updating this on my list but it drifted to the bottom...

I think I've addressed all feedback apart from the m_lldbDetached variable Kevin objected to. I'm open to suggestions how to make that aspect less eyebrow-raising. Give it a more generic name and add a comment that it's only set/used for lldb (or else set it in every backend)?

rjvbb updated this revision to Diff 46025.Nov 22 2018, 4:51 PM

Refactored for the standalone DrKonqi repo and disabled the integration testing on Mac.

Making DrKonqi standalone is a good step, I'd strongly suggest to move it to KF5 Applications at the first possible occasion. The utility doesn't even depend on a single Plasma library and provides a service that has nothing Plasma-desktop specific.

Instead, ask yourself if automatic crash reports are welcome only from Plasma desktop users or if as many users as possible should be able to submit crash reports (i.e. from any platform where DrKonqi is functional). Better, don't ask yourself, ask the entire family of KDE developers.

On a related note: DrKonqi's dependencies have been bumped along with the other Plasma dependencies. That's overkill: it has no business requiring Qt 5.11, 5.9LTS provides all required APIs. Similarly, it builds just fine against KF5 Frameworks 5.47.0, possibly even earlier versions.

rjvbb retitled this revision from DrKonqi : lldb support to DrKonqi : lldb and Mac support.Nov 22 2018, 4:54 PM
rjvbb edited the summary of this revision. (Show Details)
rjvbb set the repository for this revision to R871 DrKonqi.
rjvbb edited subscribers, added: KDE Applications; removed: plasma-devel.

I agree, with your points.
About the mac compatibility, kcrash and drkonqi (It was merged yesterday) are already in KDE-mac brew repository, looking forward for this to be accepted !
I'll try to do some tests with your patch here today.

rjvbb added a comment.Nov 22 2018, 5:03 PM

FWIW, anyone can request changes to a review, but also accept it. And while I would probably not take that as a green light to commit unless it comes from a known KDE dev it *does* go to show demand/need for a change. And hopefully, confirmation that it doesn't only work for the author.

Can you rebase it over the last master branch ?

rjvbb added a comment.Nov 22 2018, 6:58 PM
Can you rebase it over the last master branch ?

I did that hours ago, or at least I thought I did?! Is there a commit later than 62c33ba3a885106f31706cbfecc75190ca00c70c which I somehow do not see yet?

After taking a second look on it, I was able to merge the patch from the srcfolder and not from the repository root folder, weird..
Will do the tests here and now !

a find_packge for KF5::WindowSystem is missing in the root CMakeLists file.

CMake Error at src/CMakeLists.txt:84 (add_executable):
  Target "drkonqi" links to target "KF5::WindowSystem" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?

After adding the missing find_package, I was able to compile it but unable to make it to work with a test here, will do some tests to figure out the problem.

rjvbb added a comment.Nov 22 2018, 9:55 PM
a find_packge for `KF5::WindowSystem` is missing in the root CMakeLists file.

Ah, thanks. I have that in my own cmake file of course, but also an unrelated change I thought I could safely keep out by just taking a diff of the src directory.

I test DrKonqi by starting an application like kate and then doing a killall -SEGV kate.
Note that I do use a patched Qt5 on Mac where QStandardPaths can be made to behave like it does on any other Unix variant. Without that DrKonqi might well be unable to find the lldbrc (or equivalent) file it needs.

The easiest way to make this platform independent would be to put those files in the Qt resource. That's a separate change though.

rjvbb updated this revision to Diff 46044.Nov 22 2018, 10:11 PM

includes the root cmake file changes.

rjvbb set the repository for this revision to R871 DrKonqi.Nov 22 2018, 10:12 PM

Making DrKonqi standalone is a good step, I'd strongly suggest to move it to KF5 Applications at the first possible occasion.

That'll probably be when we move to Qt6.

Ultimately it'd still be the same tarball just in a different folder on an FTP site, so I doubt it really would make anyone's lives easier.


Generally this patchset is a +1 from me, except for my second comment. It's a shame we pollute the main codebase in a few places for the one backend, but if there's no other option, then we have no choice.

Can you explain why you detach? Work to just call quit in the script to solve the hanging issue?

src/aboutbugreportingdialog.cpp
39 ↗(On Diff #46044)
icon = QIcon::fromTheme
if (icon.isValid()) {
   setWindowIcon(...)
}

is more standard..

Though from the docs:

Note: On macOS, the window title bar icon is meant for windows representing documents, and will only show up if a file path is also set.

We don't set this, is this an issue?

src/backtracegenerator.cpp
139

this seems dangerous for the other clients.

It's not unfeasible for a process to have a load of data still in the buffer when it quits.

I don't know lldb, but it seems you can probably move this to ~149

rjvbb added a comment.Nov 23 2018, 8:38 AM

That'll probably be when we move to Qt6.

Which is something I hope will be as far in the future as possible :)

but

I doubt it really would make anyone's lives easier.

Moving something outside of what I secretly call the Plasma jealousy universe should make life a bit easier for those who now have to argue why they'd want to use it. I can (kind of) understand why certain Plasma code would want to use the latest Qt5 version and almost why that version would be standardised across all Plasma members even though not necessary at all. So there's that too.

Can you explain why you detach? Work to just call quit in the script to solve the hanging issue?

In a nutshell, detaching before quitting makes the chance of hanging a lot smaller and speeds up the quitting too in my experience.

As I said, I can make the variable backend-agnostic if you think that's preferable. But maybe it could be a nice junior job or GSoC project to redesign the backend so that DrKonqi doesn't have to wait for the debugger to quit. With both gdb and lldb it should be possible to obtain a reloaded backtrace from the same debugger instance, and that refreh should be a lot faster if you don't have to wait for a new debugger instance to start and churn through all loaded libraries - and I'm guessing that there will be no hanging issues when you quit lldb at DrKonqi's exit.
And if so, keeping the backend-specific variable where it is could serve as a nice little reminder.

src/aboutbugreportingdialog.cpp
39 ↗(On Diff #46044)

Hmmm, this may have changed after I developed the brunt of this patch (in KDE4 days). To answer your question, no, it's not a problem.
I'll have to look at this again, to see if this and similar changes still make sense.

src/backtracegenerator.cpp
139

You mean move the check on QProcess::Running into the if (line.startsWith(QLatin1String("Process ")) && line.endsWith(QLatin1String(" detached"))) ?

davidedmundson added inline comments.Nov 23 2018, 8:51 AM
src/backtracegenerator.cpp
139

Yes

Also is it worth adding a "break;" there?

src/data/debuggers/external/lldbrc
8

Is this AppleTerminal line meant to be here?

rjvbb added inline comments.Nov 23 2018, 9:02 AM
src/backtracegenerator.cpp
139

A break instead of or in addition to the return? I think I'd prefer the return, unless you think that maybe someday there will be some extra steps to be taken after the while loop?

src/data/debuggers/external/lldbrc
8

Yes, that file is used when the user asks to attach a debugger to the crashed process. We shouldn't presume that s/he has Konsole installed, nor that it is installed with a wrapper script on the path. So we install a script that starts Apple's standard terminal emulator with the proper arguments, and call that.

@rjvbb I got it to work here ! Took some time to figure out the problems with relative paths and KConfig, I am working to bundle kcrash in a .dmg :)

src/data/debuggers/external/lldbrc
8

Linux has lldb too

I assumed that's why you also had a "src/data/debuggers/external.mac/lldbrc" as well as this file.

rjvbb marked 3 inline comments as done.Nov 27 2018, 8:07 AM
rjvbb added inline comments.
src/data/debuggers/external/lldbrc
8

Ah, oops, good catch! Evidently the non-Mac version of lldbrc should not have AppleTerminal...

I haven't tried to understand how a debugger is chosen, I've never seen DrKonqi pick lldb over gdb on Linux. But there's FreeBSD too.

rjvbb updated this revision to Diff 46309.Nov 27 2018, 11:59 AM
rjvbb marked 2 inline comments as done.

Fixed the AppleTerminal oversight in the non-Apple lldbrc.

Also, I've removed the m_lldbDetached backend-specific member var. Instead, I now use the m_proc variable itself, and which will be nulled when we simulate the debugger exit.
Or when the debugger actuall exits when we ask it to. Sometimes it does, which is why m_proc is tested before being used when detach output has been detected.

davidedmundson accepted this revision.Nov 27 2018, 11:59 AM
rjvbb set the repository for this revision to R871 DrKonqi.Nov 27 2018, 12:00 PM
rjvbb marked an inline comment as done.

David, it seems our posts crossed, or did you actually see the new version I just uploaded?

This revision was not accepted when it landed; it landed in state Needs Review.Dec 6 2018, 9:57 AM
Closed by commit R871:13121f4a303b: lldb and Mac support (authored by rjvbb). · Explain Why
This revision was automatically updated to reflect the committed changes.
rjvbb added a comment.Dec 6 2018, 10:03 AM

This revision was not accepted when it landed; it landed in state "Needs Review".

Because I asked David if he'd seen the very last (=just committed) revision when he accepted this. He'd have infirmed that by now if our posts had crossed so I felt I could push this.

Yeah, it's all fine. Sorry should have replied when you asked.