Add Backtrace Screen #1270
Add Backtrace Screen #1270
Conversation
There was a problem hiding this comment.
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). |
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.
| 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.
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_WITHin the Autoconf manual to know what I mean.
There was a problem hiding this comment.
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.
(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.
You should guard this include line with #ifdef HAVE_LIBIBERTY.
There was a problem hiding this comment.
Coding style. Please put the conditional include lines after the mandatory/always include lines.
MrCirdo
force-pushed
the
main
branch
2 times, most recently
from
c53af06
to
792bdba
Compare
BacktraceScreen.c
Outdated
| #include "XUtils.h" | ||
| #include "errno.h" | ||
|
|
||
| #include <libiberty/demangle.h> |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
@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-libXXXwould be used. Example: In GCC's build system there are--enable-libstdcxx,--enable-libatomic,--enable-libquadmathetc. because those libraries are provided by GCC.
There was a problem hiding this comment.
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.
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.
How about just name it ENABLE_BACKTRACE?
Same wording for all --enable-* configure options in Autoconf.
There was a problem hiding this comment.
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.
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.
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.
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.
Following up internally, we'll go with:
--enable-backtraceHAVE_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.
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.
@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.
| size_t address; | ||
| size_t offset; | ||
| char* functionName; |
There was a problem hiding this comment.
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.
MrCirdo
force-pushed
the
main
branch
2 times, most recently
from
bcf3fc1
to
6b61be2
Compare
| [enable printing backtraces.])], | ||
| [], | ||
| [enable_backtrace=no]) | ||
| case "$enable_backtrace" in |
There was a problem hiding this comment.
@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])
fiThere was a problem hiding this comment.
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
configure.ac
Outdated
| [enable demangling support for backtraces @<:@default=check@:>@])], | ||
| [], | ||
| [enable_demangling=check]) | ||
| case "$enable_demangling" in |
There was a problem hiding this comment.
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])
;;
esacThere was a problem hiding this comment.
Thank you very much @Explorer09.
Your code inspired me to improve what I wrote.
There was a problem hiding this comment.
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
MrCirdo
force-pushed
the
main
branch
2 times, most recently
from
747a2c0
to
4bd4f4c
Compare
Action.h
Outdated
| @@ -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.
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.
Oh okay, I get it. It must be in c to be sure it is the first include.
README.md
Outdated
| * `--enable-iberty`: | ||
| enable the demangling support for the backtraces | ||
| - default: *no* |
There was a problem hiding this comment.
Documentation out of sync with implementation.
configure.ac
Outdated
| 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.
Isn't there some existing AutoConf magic to do this?
There was a problem hiding this comment.
I cannot use AC_CHECK_LIB because unw_get_elf_filename is a macro. I searched and it's the only method I found.
There was a problem hiding this comment.
@MrCirdo Use AC_LINK_IFELSE then, and add a comment that unw_get_elf_filename may be a macro.
There was a problem hiding this comment.
It's done. Thank you @Explorer09
There was a problem hiding this comment.
No no no. The reason for AC_LINK_IFELSE is that it checks the linking with the library as well as compiling. So you should write a simple program that calls the function (be it a macro or not). You can use LDFLAGS and LIBS variables along with AC_LINK_IFELSE.
There was a problem hiding this comment.
Oh okay, I see. If I understand correctly. I wrote that :
if test "x$enable_backtrace" = xunwind-ptrace; then
# Check if the macro unw_get_elf_filename exists
AC_LINK_IFELSE(
[AC_LANG_PROGRAM(
[[#include <libunwind-ptrace.h>]],
[[unw_get_elf_filename(0, 0, 0, 0);]]
)],
[AC_DEFINE([HAVE_LIBUNWIND_ELF_FILENAME], [1], [Define if libunwind has unw_get_elf_filename])],
[]
)
fi
It works but is it correct?
There was a problem hiding this comment.
@MrCirdo You partly get the idea. This checks if the macro exists and whether it can link to libunwind successfully with the said function.
Allow me to polish the code a bit (I add a AC_MSG_CHECKING so that when the compiling fails, it's easier to look up the error in the config log file; also I add type casts which might be helpful):
if test "x$enable_backtrace" = xunwind-ptrace; then
AC_MSG_CHECKING([if libunwind has unw_get_elf_filename])
AC_LINK_IFELSE(
[AC_LANG_PROGRAM(
[[/* unw_get_elf_filename might exist as a macro */
#include <libunwind-ptrace.h>]],
[[unw_get_elf_filename((unw_cursor_t*)0, (char*)0, (size_t)0, (unw_word_t*)0);]]
)],
[AC_DEFINE([HAVE_LIBUNWIND_ELF_FILENAME], [1], [Define if libunwind has unw_get_elf_filename])
AC_MSG_RESULT(yes)],
[AC_MSG_RESULT(no)]
)
fiThere was a problem hiding this comment.
Thank you very much @Explorer09, I added your code 😄
I pushed some changes about the demangling (strongly inspired by your code). Is it good for you?
configure.ac
Outdated
| [enable_demangling=no]) | ||
| case "$enable_demangling" in | ||
| check|yes|libiberty) | ||
| enable_demangling=libiberty |
There was a problem hiding this comment.
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.
configure.ac
Outdated
| 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.
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.
MrCirdo
force-pushed
the
main
branch
4 times, most recently
from
dbea72f
to
07f612d
Compare
Co-authored-by: Benny Baumann <[email protected]> Co-authored-by: Kang-Che Sung <[email protected]>
Co-authored-by: Benny Baumann <[email protected]> Co-authored-by: Kang-Che Sung <[email protected]>
Co-authored-by: Benny Baumann <[email protected]> Co-authored-by: Kang-Che Sung <[email protected]>
Co-authored-by: Benny Baumann <[email protected]> Co-authored-by: Kang-Che Sung <[email protected]>
Co-authored-by: Benny Baumann <[email protected]> Co-authored-by: Kang-Che Sung <[email protected]>
|
This is what it looks like now : |
|
As you already have the various fields separated it would be nice to show this kinda tabulated. Also having Also with long filenames (especially on Nix) it would be nice to have this vie care about the "hide program path" setting and the "highlight basename"/"mark outdated" settings. |
That's a good idea!
In my original draft, it was tabulated. But I see it's a good idea. |
Depending on how much dynamic you want to implement, you could hide the file+line columns if none of the rows includes information to display for those. Sometimes you have debug information available or even a service that can pull such information on-the-fly from separate debug info caches (although such long-lasting tasks should be on-demand). Thinking of fetching debug symbols like GDB does (based on buildinfo data embedded in the binaries). But that's something for a later improvement. |
That's a good idea too. I note that.
I'm inspired by the output of So now, it's look like : I have small improvements to do and I'll push the code. Is it better now? |
|
I think if we trim leading zeros for
The Okay, now the questions from my side:
|
Yes there are a lot of zeros because my computer is 64-bit, so the address has 16 digits. However, you point out something interesting. What if htop is running on a 16-bit processor? And it's a good idea to have a dynamic column to handle this situation. Now I have multiple options to make this column dynamic. There is the right alignment (as you said @BenBE) but there is also zero-padded dynamic alignment. Let me try both.
Yes it's the offset in the functions. It's a good idea to show it with a hex representation. Fun fact
I'm concerned about the size of the name of this column. Because generally, there are less than 100 frames. And I can put
Euh no, I like colors so I tried to show the color that renders more beautiful and more visible.
Yes, I can try to see what it would look like. In the
Euh no, but I'm not sure what do you want to say?
Yes It's definitely better. |
Nope. htop doesn't support any 16-bit operating system at the moment, and it's unlikely to ever happen. You might be referring to 32-bit instead.
The principle is, if there is any special information conveyed through color alone, it should be documented, and provide an alternative representation for monochrome terminals (in other words, think of colorblind accessibility). |
We always can just call that column
If you do not have special meaning for a color you shouldn't be using one unnecessarily. It's not about being as colorful as possible, but to convey information in a concise and structured way.
Have a look at the options |
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-stackprovided by elf-utils.The standard output is parsed and printed to the screen.
Currently, I have implemented only the
Refreshbutton. And my world is inspired byTraceScreenandOpenFilesScreen.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?