D28129: Read the new message string after rather than before
This revision was automatically updated to reflect the committed changes. Closed by commit R871:cfa43ed278c8: Read the new message string after rather than before (authored by apol). REPOSITORY R871 DrKonqi CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28129?vs=78627=78628 REVISION DETAIL https://phabricator.kde.org/D28129 AFFECTED FILES src/backtracegenerator.cpp src/data/debuggers/internal/gdbrc src/data/debuggers/internal/lldbrc src/debugger.cpp src/debugger.h src/parser/backtraceline.h src/parser/backtraceparser.cpp src/parser/backtraceparser.h src/parser/backtraceparser_p.h src/parser/backtraceparsergdb.cpp src/parser/backtraceparsergdb.h To: apol, #frameworks, broulik, sitter Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28129: Read the new message string after rather than before
apol updated this revision to Diff 78627. apol added a comment. address weird +2 in the code REPOSITORY R871 DrKonqi CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28129?vs=78605=78627 BRANCH arcpatch-D28129 REVISION DETAIL https://phabricator.kde.org/D28129 AFFECTED FILES src/backtracegenerator.cpp src/data/debuggers/internal/gdbrc src/data/debuggers/internal/lldbrc src/debugger.cpp src/debugger.h src/parser/backtraceline.h src/parser/backtraceparser.cpp src/parser/backtraceparser.h src/parser/backtraceparser_p.h src/parser/backtraceparsergdb.cpp src/parser/backtraceparsergdb.h To: apol, #frameworks, broulik, sitter Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28129: Read the new message string after rather than before
sitter accepted this revision. sitter added a comment. Mind the comment about the +2 please. Other than that looks reasonable. INLINE COMMENTS > backtraceparsergdb.cpp:215 > +case BacktraceLine::Info: > +d->m_infoLines << line.toString().mid(KCRASH_INFO_MESSAGE.size() + > 2); > +break; Please add a comment on what that +2 is, or better yet give it a var so it has an explicit name in code. REPOSITORY R871 DrKonqi BRANCH arcpatch-D28129 REVISION DETAIL https://phabricator.kde.org/D28129 To: apol, #frameworks, broulik, sitter Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28129: Read the new message string after rather than before
apol updated this revision to Diff 78605. apol added a comment. Don't leak REPOSITORY R871 DrKonqi CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28129?vs=78604=78605 BRANCH arcpatch-D28129 REVISION DETAIL https://phabricator.kde.org/D28129 AFFECTED FILES src/backtracegenerator.cpp src/data/debuggers/internal/gdbrc src/data/debuggers/internal/lldbrc src/debugger.cpp src/debugger.h src/parser/backtraceline.h src/parser/backtraceparser.cpp src/parser/backtraceparser.h src/parser/backtraceparser_p.h src/parser/backtraceparsergdb.cpp src/parser/backtraceparsergdb.h To: apol, #frameworks, broulik, sitter Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28129: Read the new message string after rather than before
apol updated this revision to Diff 78604. apol marked 3 inline comments as done. apol added a comment. Rebase on top of ksyntaxhighlighting, works great REPOSITORY R871 DrKonqi CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28129?vs=78117=78604 BRANCH arcpatch-D28129 REVISION DETAIL https://phabricator.kde.org/D28129 AFFECTED FILES src/backtracegenerator.cpp src/data/debuggers/internal/gdbrc src/data/debuggers/internal/lldbrc src/debugger.cpp src/debugger.h src/parser/backtraceline.h src/parser/backtraceparser.cpp src/parser/backtraceparser.h src/parser/backtraceparser_p.h src/parser/backtraceparsergdb.cpp src/parser/backtraceparsergdb.h To: apol, #frameworks, broulik, sitter Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28129: Read the new message string after rather than before
sitter added a comment.
Mh. Not quite what I had in mind but I suppose it makes sense this way.
I think we need a test case for the highlighter though :| It totally blows up
in my face when I trace a running dolphin because toskip isn't quite right.
INLINE COMMENTS
> backtracegenerator.cpp:95
>
> +auto preamble = new QTemporaryFile;
> +preamble->open();
This is leaking the file, is it not? It never deletes this object.
> gdbhighlighter.cpp:59
> int lineNr = currentBlock().firstLineNumber();
> +int toskip = 1 + m_infoLinesCount; // 1st line contains 'Application:
> ...'
> while ( cur < text.length() ) {
lineNr is initialized to currentBlock().firstLineNumber() that is not
necessarily 0, so toskip needs to be
`lineNr + 1 + infocount` otherwise the skipping doesn't work as expected.
Should also be camel toSkip.
> gdbhighlighter.cpp:65
> }
> -if (lineNr == 0) {
> -// line that contains 'Application: ...'
> +if (lineNr <= toskip) {
> ++lineNr;
Isn't this off-by-one versus the original code?
Say we have no infolines we'd then skip
[0] and [1] when previously we'd only skip [0].
> gdbhighlighter.cpp:76
> +// toskip since we skip the first line and the info lines
> +QMap< int, BacktraceLine >::iterator it = lines.lowerBound(lineNr -
> toskip);
> Q_ASSERT(it != lines.end());
The assert below fails when I trace a running dolphin, I am not super sure why
but I am guessing it's because the toskip init being bugged vis a vis the
lineNr being an offset.
REPOSITORY
R871 DrKonqi
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D28129
To: apol, #frameworks, broulik, sitter
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus,
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28129: Read the new message string after rather than before
apol updated this revision to Diff 78117. apol added a comment. This revision is now accepted and ready to land. Fix it in a theoretically better but more convoluted way REPOSITORY R871 DrKonqi CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28129?vs=77929=78117 BRANCH master REVISION DETAIL https://phabricator.kde.org/D28129 AFFECTED FILES src/backtracegenerator.cpp src/backtracewidget.cpp src/data/debuggers/internal/gdbrc src/data/debuggers/internal/lldbrc src/debugger.cpp src/debugger.h src/gdbhighlighter.cpp src/gdbhighlighter.h src/parser/backtraceline.h src/parser/backtraceparser.cpp src/parser/backtraceparser.h src/parser/backtraceparser_p.h src/parser/backtraceparsergdb.cpp src/parser/backtraceparsergdb.h To: apol, #frameworks, broulik, sitter Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28129: Read the new message string after rather than before
sitter added a comment. Quick recap from what we talked about on telegram: putting the print after the bt is most definitely going to throw off the backtrace parsing logic, so doing it this way would require extensive changes there, which is a dangerous place to make extensive changes. Or we could define a simple function to ignore errors https://stackoverflow.com/questions/17923865/gdb-stops-in-a-command-file-if-there-is-an-error-how-to-continue-despite-the-er but that's also a bit faffy. Another completely standalone approach would be to change BacktraceGenerator (I think?) to invoke the print in a completely independent gdb invocation i.e. separate the print call from the regular batchcommands and have drkonqi assemble it back into the final report. That way the actual tracing batch command couldn't fail on the print. REPOSITORY R871 DrKonqi REVISION DETAIL https://phabricator.kde.org/D28129 To: apol, #frameworks, broulik, sitter Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28129: Read the new message string after rather than before
apol planned changes to this revision. apol added a comment. Just tested it properly, it fails to generate a backtrace altogether :( REPOSITORY R871 DrKonqi REVISION DETAIL https://phabricator.kde.org/D28129 To: apol, #frameworks, broulik, sitter Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28129: Read the new message string after rather than before
broulik accepted this revision. broulik added a comment. This revision is now accepted and ready to land. <3 REPOSITORY R871 DrKonqi BRANCH master REVISION DETAIL https://phabricator.kde.org/D28129 To: apol, #frameworks, broulik, sitter Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28129: Read the new message string after rather than before
apol created this revision. apol added reviewers: Frameworks, broulik, sitter. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. apol requested review of this revision. REVISION SUMMARY This way if we're linking against an old KF5, we still generate a backtrace REPOSITORY R871 DrKonqi BRANCH master REVISION DETAIL https://phabricator.kde.org/D28129 AFFECTED FILES src/data/debuggers/internal/gdbrc src/data/debuggers/internal/lldbrc To: apol, #frameworks, broulik, sitter Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
