Improve backtrace #5312
Merged
Improve backtrace #5312
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Just tried it out, by introducing a segfault into the plugin execution code. It's awesome! I get the exact line number in the code where the segfault happened in the "better st(r)acktrace". That's huge for debugging! Great job! I'm wondering though, why are the lines like Maybe the numbering could also be fixed, so that the original stacktrace and the better stacktrace both begin counting with 1. |
|
@rolandlo I think we want to merge this into 1.2 since it is mostly a bugfix. my approach refactores that, and replaces it with std::stacktrace. |
|
?? ??:0 is printed when nothing is found at the given address in the given file (because it's not from the xournalpp executable). It can be improved a little by passing the correct executable as an argument, but not by much. Compare: Details
using using Should we make a case distinction for that (use addr2line for xournalpp calls and eu-addr2line for library calls)? I hadn't seen your work @Febbe. It looks very good as well. So your idea would be to put this PR into the next minor release and change to yours when it'll be done? |
I agree that this should be rebased onto |
From the example that would make sense. How does this work out if you run |
Febbe
requested changes
Nov 4, 2023
There should be something else which is wrong.
Yes your approach just improves/fixes the current behavior which has been broken by ASLR. My approach adds stacktraces to all OS's, by replacing the stacktrace backend, which is a refactoring. |
|
libbacktrace looks very easy to use, but I can't figure out how to have it link correctly with cmake (I'm a complete novice to cmake). Also this would imply an additional dependency to build along with Xournal++ if I understand this right? |
Closed
Yeah, just was an idea, I would like to see the simpler solution from yourself merged. |
tmoerschell
force-pushed
the
improve-backtrace
branch
from
fd7823d
to
c759d83
Compare
|
Ok, so since we have to look up the object base load address anyways, we might as well use it directly up front, with no edgy text parsing. The pointer arithmetic isn't very pretty and if someone finds a better way to do that it'd be wonderful. There is still a difference between Note that |
I would move it to |
I think, I've already did that in my PR. |
So does that mean no need to do it here? |
Either that, or that you should move it exactly like I did. |
Febbe
reviewed
Nov 6, 2023
I can confirm that the current version works fine with self-built debug libraries. See this example: Crash log when preloading gtk, gdk, goject, glib and gio |
Febbe
approved these changes
Nov 7, 2023
|
@rolandlo thank you for the visual testing. |
Do you mean squash? |
Yes :D |
tmoerschell
force-pushed
the
improve-backtrace
branch
from
3a8ff77
to
e9d10eb
Compare
|
@rolandlo when you are also fine with it, you can merge it directly, I'll do it otherwise in the late evening MET . |
rolandlo
reviewed
Nov 8, 2023
tmoerschell
force-pushed
the
improve-backtrace
branch
from
e9d10eb
to
75da72c
Compare
|
Thank you @tmoerschell, this hopefully helps to find more bugs and crashes. |
|
It's always a pleasure to contribute! Thanks to you guys! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed that the "better stacktrace" in the error logs was just the same as the original one. I found out that the argument passed to
addr2linewasn't correct (it expects the section offset, not the runtime function address).I used a regex to extract the section offset.
I set
addr2lineto output demangled function names (-fC+pto write in on one line).Even found a memory leak in the function, which I couldn't just let slide.
This will now print source files and line numbers for calls that are from within the xournalpp executable. I haven't found out how to add information for external libraries, should debug information be provided (or at least how to demangle the function names).
I'm open to formatting changes. I didn't want to make too many changes, but if we have the source call location for a stack frame, I'd say we could drop the original message (which is still printed out in the log file anyways).
I hope this can be useful!