New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Backtrace Screen #1270
base: main
Are you sure you want to change the base?
Add Backtrace Screen #1270
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your PR. The feature itself looks interesting and fits quite nicely with existing functionality.
But, the use of external tools is a bit problematic and is best to be avoided. For the case of retrieving stack traces, there are several libraries available, that might be worth a look (e.g. libunwind
). Also I'm not sure if the code as-is would properly run on e.g. FreeBSD or Darwin. Thus I strongly prefer a solution that allows to split out these platform-dependent parts where necessary (in the case of lsof
, things are portable enough across all our target platforms so it's not an issue there; not sure about eu-stack
though).
While reading through your PR I noticed that this seems to handle debug information. As such, it would be nice to have the module, source file and line be available separately (where available). Also the setting of hiding path names should be respected for module filenames in order to be consistent with the rest of the UI. Taking the highlight basename setting into account would be nice too.
Another code refactoring task is related to a recent addition of the generalized columns code that @natoscott recently worked on. Please have a look there if you like.
Also there's a few further notes regarding the code which you can find below. Please feel free to rebase the fixes to those issues directly into the existing commits as you see fit.
If you need further assistance feel free to ask.
Thank you for your reactivity and your review. I agree with you when you say Moreover, I think also the library I saw very quickly the work of @natoscott and I will wait until his work is finished. |
You're welcome.
There seems to be some patches re FreeBSD, but I'd not actually call this support … ;-) That's with Darwin aside entirely … ;-)
Sure, go ahead. Maybe check for similar places elsewhere in case something similar was missed in other places of this PR.
If you prepare your PR to anticipate that change, e.g. by preparing the data structures in a way that makes this move easy, you could even prepare things now. Given the remaining aspects in that other PR will take some time to sort out, don't feel obliged to wait for those changes to land. If the structures introduced in this PR are clean enough there's not too much work later to move things over to this new interface. Most of the work actually has already been done. |
Please don't merge the |
The flake commit is just for me. I will remove it at the end. |
Hey, I didn't have a lot of time recently for this PR. But I have some questions.
I'm looking for |
The |
Thank you very much for the clarification. I was not sure I would be able to modify it (I mean If my changes would be accepted). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please no custom build system files from e.g. Flake …
BacktraceScreen.c
Outdated
for (size_t i = 0; i < backtraceScreen->nbBacktraces; i++) { | ||
Backtrace *backtrace = &backtraceScreen->backtraces[i]; | ||
char *tgidStr = NULL; | ||
if (!isThread) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!isThread) { | |
if (!isThread) { |
Hi @BenBE , |
Don't worry. No need to apologize. The PR is marked as draft anyway, thus the only things I remarked upon was what I noticed immediately. I also ticked off some of the previous comments that no longer apply to the current state of this PR. Basically some maintenance. NB: This PR would be scheduled for 3.4.x earliest anyway given that 3.3.0 is about to ship anytime soon. |
Hi, This PR is ready for review. Currently, I add only the support of Linux. |
configure.ac
Outdated
@@ -561,6 +561,39 @@ case "$enable_unwind_ptrace" in | |||
;; | |||
esac | |||
|
|||
AC_ARG_ENABLE([libiberty], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autoconf naming convention typically avoids "--enable-LIBRARY" for library support. You have two ways to fix this:
- Use "--enable-FEATURE" naming such as "--enable-demangle". I personally recommend this way as people can add demangling support for other OSes.
- Change the option name to "--with-libiberty". See
AC_ARG_WITH
in the Autoconf manual to know what I mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I choose the first one. I let you solve the conversation if the change is correct for you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Please help me close this conversation. I cannot close it myself and there's another conversation below this one. Thus this conversation can be closed as resolved/ outdated.)
BacktraceScreen.c
Outdated
#include "XUtils.h" | ||
#include "errno.h" | ||
|
||
#include <libiberty/demangle.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should guard this include line with #ifdef HAVE_LIBIBERTY
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, See!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding style. Please put the conditional include lines after the mandatory/always include lines.
c53af06
to
792bdba
Compare
BacktraceScreen.c
Outdated
#include "XUtils.h" | ||
#include "errno.h" | ||
|
||
#include <libiberty/demangle.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding style. Please put the conditional include lines after the mandatory/always include lines.
BacktraceScreen.c
Outdated
|
||
const pid_t pid = Process_getPid(this->process); | ||
|
||
if (pid == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional to compare (pid == 0)
only or is it (pid <= 0)
? (PID is a signed type. Negative PIDs are reserved for references to process groups in most Unix-like systems.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, If I understand correctly what you said, a negative PID can represent a group of processes, but can Process_getPid
return a group of processes (and return a negative pid)?
But It makes sense to attach to one process/thread and not a group of processes, so it should be pid <= 0
.
BacktraceScreen.h
Outdated
char* functionName; | ||
bool isSignalFrame; | ||
|
||
bool isError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite get the idea why you need this "isError" boolean flag. Is it not enough to check whether the "error" pointer is NULL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't entirely agree with what I did about error management. I mean, the structure Frame
should contain only data associated with the frame. And It doesn't make sense for me to have a field error
representing a general error on a frame
. I have to try other ways to show an error.
configure.ac
Outdated
@@ -561,6 +561,37 @@ case "$enable_unwind_ptrace" in | |||
;; | |||
esac | |||
|
|||
AC_ARG_ENABLE([iberty], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. libiberty
isn't just about demangling. libiberty
is a generic support library for the GNU toolchain in which demangling is one of the main features. I said to name the keyword after the feature, that is, "--enable-demangle
". Maybe we can allow specifications like --enable-demangle=libiberty
in the future if we support other library/API for the demangling purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MrCirdo Just to give you an idea: When an optional feature introduces a library dependency, we name the "--enable-
" option after the feature, not after the library. For example the --enable-unicode
option in htop and not --enable-libncursesw
(it's not --enable-ncursesw
either). The "--with-
" option naming is to specify dependencies, thus --with-libncursesw
would make sense.
- For software that provides the library rather than depend on them,
--enable-libXXX
would be used. Example: In GCC's build system there are--enable-libstdcxx
,--enable-libatomic
,--enable-libquadmath
etc. because those libraries are provided by GCC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the explanation @Explorer09. I will try one more time 😄
Given that this feature will be available on multiple platforms and requires changes in different places I'd suggest to have a top-level feature flag for backtrace support and additional flags for the various libraries (iberty, eu-stack, …) The actual implementation for the stack traces should go in the platform code for each system, leaving the actual screen implementation mostly platform independent. |
Ok I see what you mean, is it a good idea to be inspired by for example |
Maybe a better candidate may be the way in which |
Okay thank you, I'll take a look |
configure.ac
Outdated
;; | ||
esac | ||
if test "x$enable_backtraces" = xunwind-ptrace; then | ||
AC_DEFINE([BACKTRACE_ENABLED], [1], [Define if the backtraces is enabled]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer BACKTRACE_SUPPORT
. Maybe others have other suggestions?
The name BACKTRACE_ENABLED
somehow does not really convey what that switch is about. Also it might be mistaken for some misnamed --enable-???
flag where naming got mixed up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just name it ENABLE_BACKTRACE
?
Same wording for all --enable-*
configure options in Autoconf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And by the way, I don't like the idea of using plural for the "--enable-backtraces
" option. The term "backtrace" may be used as an adjective or a verb, and the singular/infinitive form is more widely seen in web searches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer also BACKTRACE_SUPPORT
. I'll change.
For the plural of backtrace
, I also agree with you @Explorer09. --enable-backtrace
can mean enable the support of backtrace, so it's singular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any objections to --enable-backtrace-support
then? I know it's kinda long, but it avoids the issue with postfixing the enable
…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think --enable-backtrace
is best as we don't do --enable-unwind-support
etc. either
The macro names seem to be commonly HAVE_*
. I would let @BenBE do the final call here as he is typically closest to the coding style.
Greets
/DLange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up internally, we'll go with:
--enable-backtrace
HAVE_BACKTRACE
Though I'm not fully happy with this, this is, based on discussions, the most consistent way to go forward in this case IMO. It also matches best with what we already use for other feature flags in our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just --enable-backtrace
or --enable-backtrace=unwind-ptrace
? I'm confused.
It makes sense to implement --enable-backtrace=unwind-ptrace
. The methods could be different for the other platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MrCirdo Implement both. Let --enable-backtrace
translate to --enable-backtrace=unwind-ptrace
in Linux, and --enable-backtrace=no
for other platforms. In case you don't understand how to use Autoconf macros, feel free to ask and I can offer help when I can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, I see. I'll try.
size_t address; | ||
size_t offset; | ||
char* functionName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably this should split the library from the function name as separate fields. Also, if available through debug symbols, file+line number information are commonly useful.
bcf3fc1
to
6b61be2
Compare
[enable printing backtraces.])], | ||
[], | ||
[enable_backtrace=no]) | ||
case "$enable_backtrace" in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MrCirdo
Code simplification and bug fix:
- Since you are going to link to both libunwind-ptrace and libunwind-generic, it is unnecessary to use two AC_CHECK_LIB macros and the way you wrote it won't work.
AC_CHECK_HEADERS([libunwind-ptrace.h], ...)
andAC_CHECK_HEADERS([libunwind/libunwind-ptrace.h], ...)
would define different C macros. It doesn't work the way you think. I correct it so that it works the way you intended.
if test "x$enable_backtrace" = xyes; then
case $my_htop_platform in
linux)
enable_backtrace=unwind-ptrace
;;
*)
AC_MSG_ERROR([backtrace feature not implemented for the '$my_htop_platform' platform])
;;
esac
fi
case "$enable_backtrace" in
no)
;;
unwind-ptrace)
AC_CHECK_LIB([unwind-ptrace], [_UPT_create], [], [AC_MSG_ERROR([can not find required library libunwind-ptrace and libunwind-generic])], [-lunwind-generic])
AC_CHECK_HEADERS([libunwind-ptrace.h], [], [
CPPFLAGS="$CPPFLAGS -I/usr/include/libunwind"
AC_CHECK_HEADERS([libunwind-ptrace.h], [], [AC_MSG_ERROR([can not find required header file libunwind-ptrace.h])])
])
;;
*)
AC_MSG_ERROR([bad value '$enable_backtrace' for --enable-backtrace])
;;
esac
if test "x$enable_backtrace" != xno; then
AC_DEFINE([HAVE_BACKTRACE], [1], [Define if the backtrace feature is enabled])
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, it's much clearer than what I wrote 😄
For some obscure linking error, two AC_CHECK_LIB
are needed.
Thank you very much @Explorer09
Co-authored-by: Benny Baumann <BenBE@geshi.org> Co-authored-by: Kang-Che Sung <explorer09@gmail.com>
[enable demangling support for backtraces @<:@default=check@:>@])], | ||
[], | ||
[enable_demangling=check]) | ||
case "$enable_demangling" in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My attempt to rewrite the code:
have_libiberty=no
case $enable_demangling in
check|yes|libiberty)
AC_CHECK_LIB([iberty], [cplus_demangle], [have_libiberty=yes], [
if test "x$enable_demangling" = xlibiberty; then
AC_MSG_ERROR([--enable-demangling specified but libiberty not found])
fi
])
if test "x$have_libiberty" = xyes; then
AC_CHECK_HEADERS([demangle.h], [], [
dnl FIXME: do i need to restore $CPPFLAGS after this?
CPPFLAGS="$CPPFLAGS -I/usr/include/libiberty"
AC_CHECK_HEADERS([demangle.h], [], [
have_libiberty=no
if test "x$enable_demangling" = xlibiberty; then
AC_MSG_ERROR([--enable-demangling specified but <demangle.h> not found])
fi
])
])
fi
if test "x$have_libiberty" = xyes; then
enable_demangling=libiberty
fi
;;
esac
dnl have_libXXX=no
dnl case $enable_demangling in
dnl check|yes|libXXX)
dnl AC_CHECK_LIB([XXX], [funcname], [have_libXXX=yes], [
dnl if test "x$enable_demangling" = xlibXXX; then
dnl AC_MSG_ERROR([--enable-demangling specified but libXXX not found])
dnl fi
dnl ])
dnl if test "x$have_libXXX" = xyes; then
dnl AC_CHECK_HEADERS([XXX.h], [], [
dnl have_libXXX=no
dnl if test "x$enable_demangling" = xlibXXX; then
dnl AC_MSG_ERROR([--enable-demangling specified but <XXX.h> not found])
dnl fi
dnl ])
dnl fi
dnl if test "x$have_libXXX" = xyes; then
dnl enable_demangling=libXXX
dnl fi
dnl ;;
dnl esac
case $enable_demangling in
check)
enable_demangling=no
;;
yes)
AC_MSG_ERROR([cannot find any library for the backend supported by --enable-demangling])
;;
*)
AC_MSG_ERROR([bad value '$enable_demangling' for --enable-demangling])
;;
esac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @Explorer09.
Your code inspired me to improve what I wrote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. I made a mistake in the example code given above. It should be written like this instead:
case $enable_demangling in
libiberty)
AC_DEFINE([HAVE_DEMANGLING], [1], [Define to 1 if the demangling is supported])
;;
check)
enable_demangling=no
;;
yes)
AC_MSG_ERROR([cannot find any library for the backend supported by --enable-demangling])
;;
*)
AC_MSG_ERROR([bad value '$enable_demangling' for --enable-demangling])
;;
esac
Co-authored-by: Benny Baumann <BenBE@geshi.org> Co-authored-by: Kang-Che Sung <explorer09@gmail.com>
Co-authored-by: Benny Baumann <BenBE@geshi.org> Co-authored-by: Kang-Che Sung <explorer09@gmail.com>
@@ -6,6 +6,7 @@ htop - Action.h | |||
Released under the GNU GPLv2+, see the COPYING file | |||
in the source distribution for its full text. | |||
*/ | |||
#include "config.h" // IWYU pragma: keep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go into Action.c
and other files using Action.h
. Otherwise you can't guarantee that this is the first overall included header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, I get it. It must be in c
to be sure it is the first include.
* `--enable-iberty`: | ||
enable the demangling support for the backtraces | ||
- default: *no* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation out of sync with implementation.
AC_COMPILE_IFELSE([AC_LANG_SOURCE([[ | ||
#include <libunwind-ptrace.h> | ||
#ifndef unw_get_elf_filename | ||
# error 'unw_get_elf_filename' not defined | ||
#endif | ||
]])], [AC_DEFINE([HAVE_LIBUNWIND_ELF_FILENAME], [1], [Define if libunwind has unw_get_elf_filename])], []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there some existing AutoConf magic to do this?
#include "CPUMeter.h" | ||
#include "ClockMeter.h" | ||
#include "Compat.h" | ||
#include "CPUMeter.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original sort order (binary collation) was fine … No need to touch this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll revert this change, thanks.
|
||
#if defined(HAVE_LIBUNWIND_ELF_FILENAME) | ||
unw_word_t offsetElfFileName; | ||
char elfFileName[2048] = { 0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not use MAX_PATH
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know this macro existed. I added. Thank you.
[enable_demangling=no]) | ||
case "$enable_demangling" in | ||
check|yes|libiberty) | ||
enable_demangling=libiberty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't set enable_demangling=libiberty
right now. It would affect the case...esac
logic in the next block. Set enable_demangling=libiberty
only when the check is succeeded.
The logic is this: Update the enable_demangling
value when a library is found to confirm we are going to use it. If enable_demangling
remains check
or yes
, test for another library, until there is none to test. Then, either update the enable_demangling
to no
or generate an error.
old_CFLAGS="$CFLAGS" | ||
CFLAGS="$CFLAGS -I/usr/include/libiberty" | ||
AC_CHECK_HEADERS([libiberty/demangle.h], [], [ | ||
enable_demangling=no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise. It is intentional in my code not to set enable_demangling=no
. Otherwise you won't get the error when libiberty is not found.
Hello everyone!
This is my first big pull request 😃
The goal of this PR is to add a backtrace screen for process or thread.
Here's what it looks like for a thread :
And for a process:
Behind, I use the tool called
eu-stack
provided by elf-utils.The standard output is parsed and printed to the screen.
Currently, I have implemented only the
Refresh
button. And my world is inspired byTraceScreen
andOpenFilesScreen
.I still have some work to do before my work is ready (Formatting, bug fixes, ...).
Currently, this is more of a demonstration than a cool feature 😄 .
What do you think about my work? Is it a feature that can be added?