> On July 27, 2014, 11:17 a.m., Thomas Lübking wrote: > > drkonqi/gdbhighlighter.cpp, line 74 > > <https://git.reviewboard.kde.org/r/119498/diff/1/?file=293510#file293510line74> > > > > an abort is not a crash ;-) > > > > If you hit this assert, the looked up (lineNr - 1) is somehow out of > > bounds, ie. there's no line with a key >= (lineNr -1) in the map. > > > > This should likely never happen on any system. > > Can you check what the lineNr actually is as compared to > > > > qDebug() << lineNr << lines.keys(); > > > > ? > > Ian Wadham wrote: > I did at one time have a few qDebug() statements to try and find what was > wrong with the algorithm. AFAICT it is trying to match one or more lines in > (const QString& text) with single (possibly long) lines of backtrace in > QMap<int, BacktraceLine>. I think it failed if one backtrace line was > formatted into three text lines or maybe if the last backtrace line was > formatted into two text lines. AFAICT (there is a real dearth of explanatory > comments) the code is merely introducing pretty colours, etc. into the > formatted text. As such, it should not abort Dr Konqi and lose the crash > report. > > Thomas Lübking wrote: > Of course it should not crash. > > The point is that the assert explicitly tells you that there's something > wrong with the code. > Skippping it won't fix the issu - that's like puting duct tape over a > warning sign on your cars dashboard to fix "no cooling water". > > > Since the lines map seems only used to map lines to textblocks, i assume > the issue is that the lines are eg. not "\n" terminated in gdb output on OSX > (no idea, though) and the only item in the map is that for the key "0" > > In this case that'd be the issue to fix. > > Ian Wadham wrote: > Please spare me the motoring analogies. To me, the ASSERT at this point, > is like bringing the car to a screeching halt or running it into the nearest > fence, simply because the glovebox light will not come on. I am a real-time > and O/S programmer from way back and have designed and written a few crash > procedures for different systems. They need to be rugged and simple (unlike > Dr Konqi IMHO) and to succeed no matter what. This used to be called failsafe > programming, graceful failure or graceful degradation. If an ASSERT and abort > technique has to be used in RT programming, to prevent potential database > corruption, it needs to be backed by an appropriate recovery and restart > procedure. > > Actually I think lines.end() is a legitimate return from > lines.lowerBound(lineNr - 1) in Dr K's algorithm (see > http://qt-project.org/doc/qt-5/qmap.html#lowerBound coding examples). The > lineNr can actually get ahead of the number of lines in the map by by too > much, if there are several multi-line entries in QString& text. Using an > ASSERT looks like lazy programming to me (Thinks: "I cannot see what to do in > this case, so I'll put in an ASSERT and see if it actually happens in > practice." ???). My solution is to re-use the last entry from the QMap. > Another might be to use "return;" instead of the ASSERT, leaving the last bit > of the highlighting incomplete, but not losing the valuable backtrace data. > > A better (more rugged) approach might be: > > get a line from "text" > if it is from the left-hand end of a backtrace entry (i.e. not a > continuation line) > get the next backtrace entry > if past last entry > return > re-format the line from "text" > > I would have used something like that in my patch, but I do not know > enough about the format of backtrace data to get the first "if" condition > right. How would I? > > Re "\n" characters, they occur as expected in the content of "QString& > text" on Apple OS X. > > RJVB Bertin wrote: > If it walks and talks like a crash ... we should not end up in the > discussion deadlock I once had with my boss who claimed embedded code cannot > crash (because there's no OS or whatelse to replace the application code). > > Anyway, I like Ian's suggestion to just return to the caller instead of > exitting (and I concur with his lazy programming analysis). > > So if I understood correctly, DrKonqi does some reformatting of the > backtrace before including it for upload with the bug report. What I haven't > understood is how important this reformatting is. Could it be a thought to > cancel the reformating procedure and return with the raw text, instead of > with a partly formated text, when the assert condition triggers? > > > About processing backtraces: recent OS X versions use lldb instead of > gdb. How have you tackled that, Ian - added a dependency on port:gdb ? > > Aaron J. Seigo wrote: > This is not lazy programming but programming by contract. That it reaches > the lasT of lines is a bug... it should nEver happen algorithmic Ally. The > bug appears to be lowerbound sage. It returns the next higher kety and I bet > the orig dev assumed it would return the next lower. So... perhaps che k > forst for lines.contains and if that fails the do lowerbound--. That is just > from a quick read of the code. I am not familiar with this class nor do we > have output from the affected system which would be rather helpful. > > RJVB Bertin wrote: > So the contractor was lazy, too lazy to specify how to handle this kind > of exception? ;) > > I suggested returning the raw text in part because it'd allow to upload > the offending output from affected systems ... > > (PS: Aaron, if you use an Android device for this kind of coding related > activities, you might want to turn off auto-correction and install the > Hacker's keyboard, > https://play.google.com/store/apps/details?id=org.pocketworkstation.pckeyboard) > > Thomas Lübking wrote: > @Aaron, the author seemed aware of the lowerBound behavior, see original > lines 74ff > > > The issue: > ------------- > The problem here is that the lines map is build from incoming strings. > The key indicates the linenumber of that string in \n metrics. > > If the last string starts at line i and has j > 1 lines (in terms of \n) > and the textDocument segments lines [i,i+j] into multiple lines, there's no > key >= i+k, 0 < k < j > > -> Therefore it is required to add a dummy element with the highest "l" > variable after the foreach loop in the constructor, so ::lowerBound will find > this and lines 74ff will lower the iterator. > This is different from assuming the ::end() iterator position to indicate > the post-last (what would be an option) in that it preserves the guarantee of > a sane lineNr. > > > > This is obviously not platform dependent, but a bug. > Asserts are there to detect bugs, throwing errors would probably be > nicer, but troublesome on Qt (not exception safe if you let the exception > escape to the eventloop) > > > On a general note: > ------------------- > An assert on a logic condition is either wrong in general or there's > actually a bug. Therefore it's either removed or the bug fixed. > "Fixing" by ignoring unpredicted conditions on "random" circumstances > (and the platform /is/ random in this regard) is completely unacceptable as > far as I am concerned. > > If a module causes trouble and you're running out of time it's of course > ok to skip the module and schedule the fix, but ignoring a conflict instead > of analyzing it is what /I/ was tought to consider lazy :-P > > PS: > ---- > The duct tape was btw. a reference to the Simpsons - generation gap, i > assume ;-) > > Ian Wadham wrote: > @Aaron: Welcome aboard! I think it must be Akademy 2008 in Belgium since > we last met. Nice to meet you again :-) > I have attached a log file containing (immediately before the ASSERT), > (lineNr - 1), it.key() and an abbreviation of the "text" line being processed. > > @René: Be careful who you are giving advice to... Aaron is leader of the > Plasma project :-) > > @Thomas: My sons used to watch the Simpsons. So did I, but not that > regularly. I was more into Buffy the Vampire Slayer :-) > > The ASSERT failure is definitely when the last backtrace entry has > 1 > line. See the attached file. I think adding a dummy line may be the best > solution. I still do not like using ASSERT and abort and losing the > backtrace. How about doing the same test and adding a warning message to the > outgoing backtrace text if there is any problem? Graceful failure... :-) > > Thomas Lübking wrote: > Usually an assert here would be perfectly fine, since it indicates an > unpredicted (implying "invalid" in this context) state, asserts only apply > when compiling in debug mode and since there's a bug, it's ok to take a core > dump and cause a bugreport. > > In the very specific context of DrKonqi however, i tend to agree that > aborting the crash dialog is better to be avoided in any possible case. > I'd suggest to have a QMessageBox (telling the poor soul it just hit two > bugs for one and how to report the second one) and immediately return in the > particular case. > > This is however up to DrKonqi maintainers (there multiple Q_ASSERT > calls), maybe explicitly attach Milian to this review.
@René: RE "About processing backtraces: recent OS X versions use lldb instead of gdb. How have you tackled that, Ian - added a dependency on port:gdb ?" I haven't "tackled" it. Have a heart! But I have just established that DrKonqi can accept various debuggers via KConfig files (*rc) in share/apps/drkonqi/debuggers/internal (or .../external). I know nothing about either lldb or gdb - I never use interactive debuggers as a rule. If they are sufficiently compatible with each other, maybe someone could write a DrKonqi config file for lldb. At present, DrKonqi's options are dbx, gdb and kdbgwinrc (for Windows). Or maybe MacPorts could keep gdb around "forever", as it does for other things that Apple OS X drops. - Ian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119498/#review63254 ----------------------------------------------------------- On July 30, 2014, 1:04 p.m., Ian Wadham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119498/ > ----------------------------------------------------------- > > (Updated July 30, 2014, 1:04 p.m.) > > > Review request for KDE Software on Mac OS X, KDE Runtime, kdelibs, and > Michael Pyne. > > > Repository: kde-runtime > > > Description > ------- > > When a KDE app crashes in Apple OS X, it just disappears from the screen. At > the most, the user is invited to report the crash to Apple. AFAIK this has > been a problem in KDE on Apple OS X for years, leading to frustration with > KDE among Apple users and MacPorts developers and an attitude among KDE > developers of "Why does nobody report the problem(s) on bugs.kde.org?" > > It is my strong belief that the failure to report crashes of KDE apps in > Apple OS X also exists in Frameworks. > > So far I have identified a number of portability bugs in KDE on Apple OS X: 1 > in KCrash, 1 in kdeinit4 and 5 in Dr Konqi. Three patches for Dr Konqi are > submitted in this review. Patches for KCrash and kdeinit4 are submitted in > part 1 of this review, against kdelibs. I am still investigating the other > two problems in Dr Konqi - and there could be more than two... > > In this review we have three portability problems: > > 1. On Apple OS X, Dr Konqi's dialog box hides itself underneath the main > window of the app that has just crashed, so is effectively useless. This > appears to be because Dr Konqi is started by a Linux/Unix method (fork() + > exec()?). If an app is started with the Apple OS X "open" command, it always > appears on top. The patch just raises the dialog box. > > 2. When formatting the backtrace output, Dr Konqi crashes (with an ASSERT) on > the last line. This appears to be an error in the algorithm used (i.e. also a > bug in Linux KDE), but the patch is treating it as an Apple OS X portability > problem for now. > > 3. Dr Konqi checks whether the user can save cookies in kcookiejar and, if > not, stops reporting the crash. On Apple OS X, cookies would be kept in > another browser (e.g. Safari or Firefox) and not in KDE's default browser > (Konqueror) and cookie jar. IMHO, Dr K should report the crash no matter > what, as long as it can connect to bugs.kde.org and log in. > > > Diffs > ----- > > drkonqi/reportassistantpages_bugzilla.cpp 86ca327 > drkonqi/gdbhighlighter.cpp 7cd0aa9 > drkonqi/main.cpp 75e060e > > Diff: https://git.reviewboard.kde.org/r/119498/diff/ > > > Testing > ------- > > Using Apple OS X 10.7.5 (Lion) on a MacBook Pro, I have installed KDE libs > via MacPorts (at version 4.12.5) and I have adapted kdesrc-build to run in an > Apple OS X environment and used it to test against the KDE 4.13 branch. I > have been testing with a KDE app that I can crash at will and using stderr > and Apple OS X Console log output to determine the outcome. > > Please note that I am the -only- KDE developer who has this kind of setup, > but I am NOT a KDE core developer. My experience before now has been in KDE > Games. However I used to be a UNIX and database guru before I retired in 1998. > > I NEED HELP from KDE -core- developers to proceed further. These problems > will also exist in Dr Konqi for KF 5, but I am as yet unable to build or test > Frameworks on Apple OS X and I cannot find Dr Konqi among the Frameworks > repositories. I am sure there are many more Apple OS X portability problems > in Dr Konqi and other KDE software. > > Without my patches, Dr Konqi, on Apple OS X, remains invisible to the user, > often fails to complete the backtrace report and then fails to connect to > bugs.kde.org. > > With my patches, Dr Konqi on Apple OS X can generate a full crash report, > including the backtrace and the results of the dialog with the user. > Sometimes, however, it fails to submit the completed report to bugs.kde.org. > This problem is still under investigation. > > I would not have got this far without help from Michael Pyne, Thomas Lübking > and several of the MacPorts developers, as well as the unfailing enthusiasm > and encouragement of my friend Marko Käning. > > > File Attachments > ---------------- > > Log of Dr K ASSERT problem > > https://git.reviewboard.kde.org/media/uploaded/files/2014/07/30/a3f99f00-94df-4b10-bc47-66b1c966f893__DrKonqiASSERT.kcrash.txt > > > Thanks, > > Ian Wadham > >
