Read the new message string after rather than before
ClosedPublic

Authored by apol on Mar 18 2020, 3:29 PM.

Details

Summary

This way if we're linking against an old KF5, we still generate a backtrace

Diff Detail

Repository
R871 DrKonqi
Branch
arcpatch-D28129
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24305
Build 24323: arc lint + arc unit
apol created this revision.Mar 18 2020, 3:29 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 18 2020, 3:29 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol requested review of this revision.Mar 18 2020, 3:29 PM
This revision is now accepted and ready to land.Mar 18 2020, 3:30 PM
apol planned changes to this revision.Mar 18 2020, 3:43 PM

Just tested it properly, it fails to generate a backtrace altogether :(

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.

apol updated this revision to Diff 78117.Mar 20 2020, 5:52 PM

Fix it in a theoretically better but more convoluted way

This revision is now accepted and ready to land.Mar 20 2020, 5:52 PM

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.

src/backtracegenerator.cpp
95

This is leaking the file, is it not? It never deletes this object.

src/gdbhighlighter.cpp
59 ↗(On Diff #78117)

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.

65 ↗(On Diff #78117)

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].

76 ↗(On Diff #78117)

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.

apol updated this revision to Diff 78604.Mar 27 2020, 1:32 AM
apol marked 3 inline comments as done.

Rebase on top of ksyntaxhighlighting, works great

apol updated this revision to Diff 78605.Mar 27 2020, 1:33 AM

Don't leak

sitter accepted this revision.Mar 27 2020, 11:08 AM

Mind the comment about the +2 please.

Other than that looks reasonable.

src/parser/backtraceparsergdb.cpp
215

Please add a comment on what that +2 is, or better yet give it a var so it has an explicit name in code.

apol updated this revision to Diff 78627.Mar 27 2020, 11:13 AM

address weird +2 in the code

This revision was automatically updated to reflect the committed changes.