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). |
BenBE
left a comment
BenBE
left a comment
There was a problem hiding this comment.
Please no custom build system files from e.g. Flake …
|
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. |
MrCirdo
force-pushed
the
main
branch
2 times, most recently
from
c53af06 to
792bdba
Compare
|
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 |
MrCirdo
force-pushed
the
main
branch
2 times, most recently
from
b7cbd35 to
c651b0b
Compare
| AM_CFLAGS="$AM_CFLAGS $LIBUNWIND_PTRACE_CFLAGS" | ||
| LIBS="$LIBS $LIBUNWIND_PTRACE_LIBS" | ||
| ], [ | ||
| AC_MSG_ERROR([can not find required library libunwind-ptrace and libunwind-generic]) |
There was a problem hiding this comment.
Shouldn't this only need to mention libunwind-ptrace?
There was a problem hiding this comment.
I would tend to say no, because the pkg-config of libunwind-ptrace returns both libraries.
configure.ac
Outdated
| AC_CHECK_LIB([iberty], [cplus_demangle], [ | ||
| have_libiberty=yes | ||
| AC_DEFINE([HAVE_LIBIBERTY], [1], [Define to 1 if libiberty is found.]) | ||
| LIBS="$LIBS -liberty" | ||
| ], [ | ||
| if test "x$enable_demangling" = xlibiberty; then | ||
| AC_MSG_ERROR([--enable-demangling specified but libiberty not found]) | ||
| fi | ||
| ]) |
There was a problem hiding this comment.
| AC_CHECK_LIB([iberty], [cplus_demangle], [ | |
| have_libiberty=yes | |
| AC_DEFINE([HAVE_LIBIBERTY], [1], [Define to 1 if libiberty is found.]) | |
| LIBS="$LIBS -liberty" | |
| ], [ | |
| if test "x$enable_demangling" = xlibiberty; then | |
| AC_MSG_ERROR([--enable-demangling specified but libiberty not found]) | |
| fi | |
| ]) | |
| AC_CHECK_LIB( | |
| [iberty], | |
| [cplus_demangle], | |
| [ | |
| have_libiberty=yes | |
| AC_DEFINE([HAVE_LIBIBERTY], [1], [Define to 1 if libiberty is found.]) | |
| LIBS="$LIBS -liberty" | |
| ], [ | |
| if test "x$enable_demangling" = xlibiberty; then | |
| AC_MSG_ERROR([--enable-demangling specified but libiberty not found]) | |
| fi | |
| ] | |
| ) |
There was a problem hiding this comment.
A small reminder: The order of the libraries (the -l options) specified in LIBS is significant. If library foo would call functions or otherwise depend on symbols of library bar, then -lbar -lfoo will give you a linking error. It must be -lfoo -lbar instead. The big rule is that higher level, application libraries should be specified before system libraries. The very last of the list would be -lc -lgcc, if the compiler were told not to imply these two libraries.
AC_CHECK_LIB would prepend newly checked libraries to LIBS for its default action, so the libraries checked in later AC_CHECK_LIB calls will appear earlier in the LIBS list. This follows the usual order of configure that check system libraries first, then check application level libraries.
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>
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>
Co-authored-by: Benny Baumann <BenBE@geshi.org> Co-authored-by: Kang-Che Sung <explorer09@gmail.com>
| ) | ||
|
|
||
| if test "x$have_libiberty" = xyes; then | ||
| if ! htop_search_header_dir libiberty/demangle.h "/usr/include/libiberty"; then |
There was a problem hiding this comment.
Could the header be installed in "/usr/include/libiberty/libiberty/demangle.h" (with two "libiberty" strings)? Really?
Also, which name is preferred for the demangle header when specified in the #include directives, <demangle.h> or <libiberty/demangle.h>?
There was a problem hiding this comment.
Could the header be installed in "/usr/include/libiberty/libiberty/demangle.h" (with two "libiberty" strings)? Really?
Hum.... I don't think... However, it works too when I tested :
htop_search_header_dir libiberty/demangle.h "I'm spider man"
This is why the configuration succeeded on my machine.
Also, which name is preferred for the demangle header when specified in the #include directives, <demangle.h> or <libiberty/demangle.h>?
<demangle.h> doesn´t work on my machine, only <liberty/demangle.h> works. I can test on the CI if you want.
I propose
htop_search_header_dir libiberty/demangle.h "/usr/include/"
What do you think?
There was a problem hiding this comment.
/usr/include should be in the search path by default.
There was a problem hiding this comment.
Could the header be installed in "/usr/include/libiberty/libiberty/demangle.h" (with two "libiberty" strings)? Really?
Hum.... I don't think... However, it works too when I tested :
htop_search_header_dir libiberty/demangle.h "I'm spider man"This is why the configuration succeeded on my machine.
The htop_search_header_dir function would not try your I'm spider man directory when the header can be found in the default include directories. This behavior is by design.
The proper way to call the htop_search_header_dir function is to first find out the preferred way to specify the header name in the #include directive and use that as the first argument, and that's why I asked this question:
Also, which name is preferred for the demangle header when specified in the #include directives, <demangle.h> or <libiberty/demangle.h>?
Some headers are preferred to be included with subdirectory name, for example POSIX <sys/types.h> header (never include it as <types.h>!)
For the cplus_demangle function, according to these two man pages, the preferred include name is <demangle.h> and NOT <libiberty/demangle.h>! So, you should call htop_search_header_dir with demangle.h as the first argument. Then…
<demangle.h>doesn´t work on my machine, only<liberty/demangle.h>works. I can test on the CI if you want.
That's what the second argument of htop_search_header_dir comes in handy! The second argument specifies additional search directories where the header might be found. In your case, mention "/usr/include/libiberty" as the second argument and your include can work again!
The second argument allows a list of directories (separated by space) to search for the header, because different operating system might install the demangle.h in different locations. The locations could be
/usr/include/demangle.h
/usr/include/libiberty/demangle.h
Or even /opt/my_awesome_software/libiberty/include/demangle.h. (Okay, this last case is made up, but you get the idea.)
So a list of search directories should be specified there as the second argument.
So, in summary, (1) use
htop_search_header_dir demangle.h "/usr/include/libiberty"in the configure script, and (2) use #include <demangle.h> in all the C files that call cplas_demangle function.
There was a problem hiding this comment.
|
There are three things I wish to be done for your PR:
// libiberty (GNU toolchain)
// <https://codebrowser.dev/gcc/libiberty/cplus-dem.c.html#cplus_demangle>
char *cplus_demangle(const char *mangled, int options);
// libdemangle (Solaris & Oracle Developer Studio)
// <https://docs.oracle.com/cd/E57201_01/html/E57229/cplus-demangle-3.html>
int cplus_demangle(const char *symbol, char *prototype, size_t size);That's a potential for extending the demangling feature to other libraries, but that means a more precise configure check is needed. The demangling code should also be placed in a platform-independent location (since this depends on compiler tools / libraries and not operating system).
|
|
That actually means, the |
You mean P.S. I'm trying to write the configure check code for libunwind-ptrace, as I believe I can write the configure code better than what @MrCirdo can come up with (libunwind-ptrace depends on libunwind, and so the pkg-config code of both can be slightly complicated). |
I think
Well, if PCP needs something else for their backtrace implementation, they just do not include that file as part of their build and instead implement their interface in their local platform code. Similar to how most *BSDs us the
No reason to over-complicate things. If we have a working version for the first round, we can iterate on that later. Priority is on getting things work; polishing comes later. |
| char* demangledName = cplus_demangle(frame->functionName, | ||
| DMGL_PARAMS | AUTO_DEMANGLING); |
There was a problem hiding this comment.
It took me quite a bit of code digging that I found out that the use of AUTO_DEMANGLING here is wrong.
| char* demangledName = cplus_demangle(frame->functionName, | |
| DMGL_PARAMS | AUTO_DEMANGLING); | |
| char* demangledName = cplus_demangle(frame->functionName, | |
| DMGL_PARAMS | DMGL_AUTO); |
There was a problem hiding this comment.
Very good catch! AUTO_DEMANGLING is for Some macros to test what demangling style is active. So yes better to use DMLG_AUTO
There was a problem hiding this comment.
Can you tell me the rationale of why you use these demangle option flags and not others?
There are 6 boolean DMGL_* flags defined in the header as well as the language. I'm pretty sure you wanted DMGL_AUTO for the language choice, but what about the other flags?
DMGL_PARAMS /* Include function args */
DMGL_ANSI /* Include const, volatile, etc */
DMGL_VERBOSE
DMGL_TYPES
DMGL_RET_POSTFIX
DMGL_RET_DROP
This was my concern when @BenBE suggested supporting the other platform.
What I propose to you (because all the code from the configuration came from you 😄) is to split the PR into two parts. The first is the configuration of the libraries |
You sound like I haven't made a 1000 line PR before. No offence, but look at #1415, which I didn't imagine it would be that big until I finished it (but I'm happy that I've done a big feature). Anyway, the newer suggestion is to put libunwind-ptrace code into the
No need to worry that part because I've written the configure check for it, and it can detect
I'm sorry, I probably missed something. What did you propose to me? |
We've all been there … cough #305 cough #388 cough #849
Exactly. For your PR you basically just need to add the
@Explorer09 I'll hold off with merging your PR till this one is ready. @MrCirdo You can assume that PR from @Explorer09 to be merged for your work. So if you need any further changes to Depeding on how apt you are with git, you may just rebase your PR on top of #1821. Note however, that this will likely be a moving target for the time being.
@Explorer09 You just beat him to the punch at opening the PR for rewriting the configure stuff ;-) Okay, way forward: @Explorer09 Prepare #1821 so it can serve as a good base for this PR. @MrCirdo Once @Explorer09 is ready, check if you get your changes to work with the changes in #1821. If there are any issues or if you need help, just drop a note and we'll try to assist. Meanwhile I'll take a closer look at the existing code changes in this PR and #1821 and see if there is any major notes I previously missed. I'll give you a head start for this so I won't review a moving target. ;-) |
Example in Makefile.am: if HAVE_BACKTRACE_SCREEN
myhtopheaders += BacktraceScreen.h
myhtopsources += BacktraceScreen.c
myhtopsources += generic/LibUnwindPtrace.c
if HAVE_DEMANGLING
myhtopsources += generic/Demangle.c
endif
endifI think I would add the lines (Update: I retract the idea of the Automake conditional The demangle function code is very small, and so I don't think there is a need to split files per library (
I'm fine with that. |
|
Just to mention one hellish thing: Including a libiberty header in htop can trigger a warning as htop's code enables The issue was about #ifdef HAVE_DEMANGLE_H
# if !defined(HAVE_DECL_BASENAME)
// Workaround to libiberty header that could require this to be defined.
// Suppress libiberty's own declaration of basename().
# define HAVE_DECL_BASENAME 1
# endif
#include <demangle.h>
#endif |
|
@Explorer09 Going from the GCC bug and a quick exchange I had in the GCC IRC, I'd suggest adding a comment to our configure: # libiberty drags every downstream project into their dumpster-fire of internal APIs,
# coercing users to babysit their broken exports. This was added only because they
# refuse to ship a sane library — not because we need it in our code.
#
# See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=122729
AC_CHECK_DECLS([[basename(char *)], [dirname(char *)]])As we pull in An alternative may also be: # This comment is a message... and part of a system of messages... read it.
# We deliberately left this note because libiberty imposed a burden on everyone.
# Sending this message mattered to us. We treated our build system with care.
# This is not a place of honor... no noble API is memorialized here... nothing worth celebrating.
# What is here is risky and contemptible to us. This is a warning about that risk.
# The risk is tied to a specific implementation... it worsens the closer you rely on their internals...
# the center of the problem is their library; its scope is limited, but underneath our code.
# The problem persists now, as it did when we acted.
# The problem harms the codebase, and it can break builds.
# The form of the problem is leaking internal dependencies and unstable interfaces.
# The problem is unleashed only if you depend on or use these interfaces. Best to avoid them.
#
# See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=122729
AC_CHECK_DECLS([[basename(char *)], [dirname(char *)]])(ref) Fine-tune the wording to fit if necessary. ;-) |
|
@BenBE Something technical here: My personal choice is not to add the The bad thing is that // This is UNSAFE - you can't pass a constant string literal
str1 = basename("/usr/lib/os-release");
// This is required to return a pointer to string ".", which is
// definitely an internal buffer and unsafe to modify, yet there is no
// const qualifier on the return type.
char empty[] = "";
str2 = basename(empty);I consider it kind of insane when libiberty still provides size_t dirNameLen(const char* path);
// path + ret + strlen("/") will get you the start of the basename or empty string.
char* basename_malloc(const char* path);
char* dirname_malloc(const char* path);
// The caller would be supposed to free() the returned pointer after use.Now back to the EDIT: It seems I'm not alone... (https://bugzilla.redhat.com/show_bug.cgi?id=487995) |
|
I in no way would endorse or even encourage use of an API that broken. I hope the spite in my previous comment suggestions to accompany such a configure check wasn't missed. But yes, it's kinda annoying needing to fix the broken API isolation of someone else's code. Another alternative would be to explicitly split out the That aside: We should take our liberty to confine this abomination to as little scope as possible. ACK. on your variant with defining things locally; plus if you can do a ¹pun on |
|
@BenBE I was surprised with the upstream's attitude, too, considering that the fix to the header is trivial (see the issue report for what I've proposed). And the alternative, By the way, I plan to confine that libiberty header to just in EDIT: This is the API defect I was talking about: char* Generic_Demangle(const char* mangled) {
#ifdef HAVE_LIBDEMANGLE_CPLUS_DEMANGLE
static size_t allocSize = 8;
char* buf;
while (true) {
buf = malloc(allocSize);
if (!buf)
break;
int ret = cplus_demangle(mangled, buf, allocSize)
if (ret == 0)
break;
free(buf);
buf = NULL;
if (ret != DEMANGLE_ESPACE || allocSize > SIZE_MAX / 2)
break;
allocSize *= 2;
}
return buf;
#endif
} |
No, never, in an open source project 😄
Congrats!
Okay, perfect. I'm fine with that too. I'll test its PR with mine.
I love this alternative 😆 I'm voting to implement it if it's implented! I think I understand the problem with
I read the conversation and I'm also suprised by saying |
|
Close in favor of #1821 |
Backtrace screen (improvements over #1270)
4 participants
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?