Backtrace screen (improvements over #1270) #1821

Explorer09 · 2025-11-15T21:40:48Z

This branch is experimental and related to #1270 (the backtrace screen feature).

I've written and improved the configure script so that it can check libunwind-ptrace and libiberty for the backtrace feature proposed in #1270.

The changes come with two parts:

The first part can be reviewed and cherry-picked into main early. It renames the configure option --enable-unwind to --with-libunwind to better reflect it's purpose. The old option name is preserved for compatibility but will trigger warnings when used.

The second part consists of several commits that add the new configure options --enable-backtrace and --enable-demangling and the checking code of libunwind-ptrace and libiberty. The code might look large and complicated, but I've tried a lot on the usability and correctness of the check. For example this libunwind-ptrace check can work with and without pkg-config, and I tried to ensure the cplus_demangle() function has a correct prototype.

The libunwind-ptrace and libiberty libraries can be detected both in Linux and in FreeBSD. However, I haven't ported the backtrace screen code from @MrCirdo to ensure they really build, especially in FreeBSD.

BenBE · 2025-11-16T10:00:31Z

Can you note, which commits for the first vs. second part? I assume the first commit ("Rename *" for the first and the remaining 4 later?

I think we can also do this all at once. But I'm ope on that front.

Explorer09 · 2025-11-16T16:33:14Z

Can you note, which commits for the first vs. second part? I assume the first commit ("Rename *" for the first and the remaining 4 later?

Yes, the first commit is the option rename, and the remaining 4 commits are the new feature / configure options.

The reason I suggest the rename is that --enable-backtrace=unwind-ptrace will depend on the --with-libunwind option and it's hard to split the two apart. That means the --enable-backtrace=unwind-ptrace --without-libunwind combination will being you an error.

BenBE · 2025-11-16T16:40:49Z

Do we have alternative backends for backtracing/demangling besides unwind-ptrace and iberty? Given there's both an unwind implementation in LLVM and GCC, what else are sensible alternatives that warrent implementation of the --enable-backtrace switch as --enable-backtrace=unwind (if I saw that correctly in the source)?

Explorer09 · 2025-11-16T16:51:20Z

Do we have alternative backends for backtracing/demangling besides unwind-ptrace and iberty? Given there's both an unwind implementation in LLVM and GCC, what else are sensible alternatives that warrent implementation of the --enable-backtrace switch as --enable-backtrace=unwind (if I saw that correctly in the source)?

Well, I don't know what other libraries are possible for the backtrace feature, but I leave room for those new backends.

For the demangling function, I can see libdemangle from Solaris / Sun Studio as a possible alternative, but I didn't check that yet. It's better for @MrCirdo 's code (#1270) get merged first before considering alternatives.

BenBE · 2025-11-16T17:26:38Z

NP. No need to decide on all the possible backends and variants now. That's something we can leave open for the first implementation. Priority is getting things work; make them flexible and fast is the next step. No need to over-engineer things from the get-go. I just wanted to evaluate if there's a reasonable need for allowing the backends for backtraces and demangling to be replaced.

Explorer09 · 2025-11-23T03:42:21Z

I would like to hand this branch over to @MrCirdo. I think I have done with this. After this is handed over, I have some comments on @MrCirdo 's code, things I suggest that can be written better.

Explorer09 · 2025-11-23T11:21:51Z

generic/UnwindPtrace.c

+   }
+
+   if (pid <= 0) {
+      xAsprintf(error, "Unable to get the pid");


This pid <= 0 check can be moved to before the unw_create_addr_space call. That way we can save an allocation and deallocation in case when the PID is invalid.

Yes, you're alright! Good Catch!

Explorer09 · 2025-11-23T11:27:58Z

generic/UnwindPtrace.c

+      unw_word_t pc;
+
+      BacktraceFrameData* frame = BacktraceFrameData_new();
+      frame->index = index;


Do we really need this index field in the BacktraceFrameData structure? Since the vector object of BacktraceFrameData is likely sorted by index already, we can derive the indices when showing the backtrace data.
A technical advantage of leaving out this field is that a BacktraceFrameData can then initialize to all zeros. That saves code size.

Your argument makes sense. Sometimes, when unw_get_proc_name returns an error, what I was doing was to skip the frame. However, to keep the display coherent, this index was introduced. It can be removed.

@MrCirdo In case there is a failure in processing an entry in the list (such as a process name among a list of processes), a dummy entry can be added to indicate the error to keep the index numbering sane (when displayed).

Explorer09 · 2025-11-23T11:33:07Z

generic/UnwindPtrace.c

+         if (unw_get_elf_filename(&cursor, elfFileName, sizeof(elfFileName), &offsetElfFileName) == 0) {
+            frame->objectPath = xStrndup(elfFileName, 2048);
+
+            char *lastSlash = strrchr(frame->objectPath, '/');


strrchr() can return NULL when the input string frame->objectPath contains no slashes.
This can cause the xStrndup(lastSlash + 1, 2048); to dereference an invalid pointer. We definitely need a null pointer check here.

(Thanks to the basename() API hell in libiberty that I discovered this one - an unsafe imitate of basename().)

Well See! I didn't catch this one! I'll fix it!

MrCirdo · 2025-11-23T12:30:13Z

I think I have done with this.

Great! Perfect! Thank you very much!

After this is handed over, I have some comments on @MrCirdo 's code, things I suggest that can be written better.

With pleasure!

Wait, let’s get on the same page @Explorer09.
My idea was to split my PR into parts. The first part, which is presented here, involves adding a better configuration to my original PR. The second part is the behavior of the backtrace screen. And here you split both (Deeply apologize if I explain my idea very badly). If I understand well what @BenBE said, this PR would serve as a good base for #1270. So my question is : Do I have to close #1270 and continue my work on this one? I'm confused 😄

My primary concern about supporting the BSD platform (FreeBSD, NetBSD, macOS, etc) is that I don't know if ptrace(2) has the same behavior everywhere. (I suppose yes, but I've never used a BSD platform). So, reassure me, @Explorer09, that's the case?

NB : Sorry if I take time to answer because I need to do some preparation for a coding interview.

Explorer09 · 2025-11-23T13:03:50Z

My primary concern about supporting the BSD platform (FreeBSD, NetBSD, macOS, etc) is that I don't know if ptrace(2) has the same behavior everywhere. (I suppose yes, but I've never used a BSD platform). So, reassure me, @Explorer09, that's the case?

I didn't test running the code on BSD yet. The only thing I've tested is building htop with libunwind-ptrace and libiberty on FreeBSD on a CI server. I've read the man page of ptrace(2) for FreeBSD and got the equivalent keywords (PT_ATTACH and PT_DETACH), and those are what I've done.

My aim is to make it buildable in Linux and FreeBSD; other OSes are too difficult and I would expect libunwind port to those platforms first before we try making our backtrace feature work there.

BenBE · 2025-11-24T16:53:25Z

I think I have done with this.

Great! Perfect! Thank you very much!

I.e. no further changes from @Explorer09 unless needed/requested. And possibly rebases onto main maybe.

After this is handed over, I have some comments on @MrCirdo 's code, things I suggest that can be written better.

With pleasure!

Wait, let’s get on the same page @Explorer09.
My idea was to split my PR into parts. The first part, which is presented here, involves adding a better configuration to my original PR. The second part is the behavior of the backtrace screen. And here you split both (Deeply apologize if I explain my idea very badly). If I understand well what @BenBE said, this PR would serve as a good base for #1270. So my question is : Do I have to close #1270 and continue my work on this one? I'm confused 😄

I think this PR will likely stay as is; i.e. no major changes.

My primary concern about supporting the BSD platform (FreeBSD, NetBSD, macOS, etc) is that I don't know if ptrace(2) has the same behavior everywhere. (I suppose yes, but I've never used a BSD platform). So, reassure me, @Explorer09, that's the case?

You can ask @fraggerfox regarding NetBSD details if necessary.

NB : Sorry if I take time to answer because I need to do some preparation for a coding interview.

NP.

Explorer09 · 2025-11-24T18:04:19Z

My primary concern about supporting the BSD platform (FreeBSD, NetBSD, macOS, etc) is that I don't know if ptrace(2) has the same behavior everywhere. (I suppose yes, but I've never used a BSD platform). So, reassure me, @Explorer09, that's the case?

You can ask @fraggerfox regarding NetBSD details if necessary.

The libunwind README says its supported operating systems are Linux, FreeBSD, Solaris, QNX and HP-UX (partially). NetBSD is not included.

For Solaris, there's no pre-built binary package for libunwind so testing on it would be difficult. QNX and HP-UX are out of htop's scope. So there remains Linux and FreeBSD, which are what I tried to make htop's backtrace work for.

The configure option '--enable-unwind' doesn't actually enable a feature in htop but selects a library dependency for a feature (printing local backtrace for this case). Use a '--with-' naming to reflect the option's purpose better. For backward compatibility (with automated build scripts), using '--enable-unwind' will still work but it will print a warning. Signed-off-by: Kang-Che Sung <[email protected]>

Co-authored-by: Odric Roux-Paris <[email protected]> Co-authored-by: Benny Baumann <[email protected]> Signed-off-by: Kang-Che Sung <[email protected]>

For Linux and FreeBSD build jobs.

Signed-off-by: Kang-Che Sung <[email protected]>

Co-authored-by: Benny Baumann <[email protected]>

MrCirdo · 2025-12-02T21:47:52Z

generic/Demangle.c

+
+   char* buf;
+
+   while (!!(buf = malloc(allocSize))) {


I would use realloc(3) here if the size is too small. What do you think?

Using realloc is not helpful here because the buffer may be free'd for other reasons. realloc is best used on buffers with content you need to preserve, which is not the case here.

MrCirdo · 2025-12-02T21:57:01Z

Wait, let’s get on the same page @Explorer09.
My idea was to split my PR into parts. The first part, which is presented here, involves adding a better configuration to my original PR. The second part is the behavior of the backtrace screen. And here you split both (Deeply apologize if I explain my idea very badly). If I understand well what @BenBE said, this PR would serve as a good base for #1270. So my question is : Do I have to close #1270 and continue my work on this one? I'm confused 😄

I think this PR will likely stay as is; i.e. no major changes.

Perfect! 😄 I closed the first one.

Can I push on your branch @Explorer09 to fix the problem you emphasized?

Explorer09 · 2025-12-03T02:16:14Z

Wait, let’s get on the same page @Explorer09.
My idea was to split my PR into parts. The first part, which is presented here, involves adding a better configuration to my original PR. The second part is the behavior of the backtrace screen. And here you split both (Deeply apologize if I explain my idea very badly). If I understand well what @BenBE said, this PR would serve as a good base for #1270. So my question is : Do I have to close #1270 and continue my work on this one? I'm confused 😄

I think this PR will likely stay as is; i.e. no major changes.

Perfect! 😄 I closed the first one.

Can I push on your branch @Explorer09 to fix the problem you emphasized?

I have enabled push access for this particular branch configure-enable-backtrace, so feel free to push.

Explorer09 · 2025-12-03T11:11:46Z

generic/Demangle.c

+#ifdef HAVE_DEMANGLING
+char* Generic_Demangle(const char* mangled) {
+# if defined(HAVE_LIBIBERTY_CPLUS_DEMANGLE)
+   int options = DMGL_PARAMS | DMGL_AUTO;


As mentioned in #1270, I would like to document the reasoning on the choice regarding the 6 boolean flags supported by cplus_demangle function:

DMGL_PARAMS, DMGL_ANSI, DMGL_VERBOSE, DMGL_TYPES, DMGL_RET_POSTFIX and DMGL_RET_DROP

For now I recognised why the use of DMGL_PARAMS here (necessary for C++ to distinguish function overloads), but haven't looked up on what the other 5 options do.

BenBE added enhancement Extension or improvement to existing feature code quality ♻️ Code quality enhancement build system 🔧 Affects the build system rather then the user experience labels Nov 15, 2025

BenBE added this to the 3.5.0 milestone Nov 15, 2025

MrCirdo mentioned this pull request Nov 16, 2025

Add Backtrace Screen #1270

Closed

Explorer09 force-pushed the configure-enable-backtrace branch 9 times, most recently from a964363 to 798b603 Compare November 23, 2025 01:38

Explorer09 marked this pull request as ready for review November 23, 2025 01:44

Explorer09 force-pushed the configure-enable-backtrace branch from 798b603 to 9ced4f4 Compare November 23, 2025 03:27

Explorer09 commented Nov 23, 2025

View reviewed changes

Explorer09 changed the title ~~Configure script changes for backtrace feature~~ Backtrace screen (improvements over #1270) Nov 23, 2025

Explorer09 force-pushed the configure-enable-backtrace branch 3 times, most recently from f394979 to dd9cf40 Compare November 29, 2025 17:02

Explorer09 and others added 7 commits December 3, 2025 01:17

build: Add '--enable-backtrace' configure option

026ed69

Co-authored-by: Odric Roux-Paris <[email protected]> Co-authored-by: Benny Baumann <[email protected]> Signed-off-by: Kang-Che Sung <[email protected]>

build: Add '--enable-demangling' configure option

4fc2efb

Co-authored-by: Odric Roux-Paris <[email protected]> Co-authored-by: Benny Baumann <[email protected]> Signed-off-by: Kang-Che Sung <[email protected]>

doc: Update for backtrace feature

ef6d505

Co-authored-by: Odric Roux-Paris <[email protected]> Co-authored-by: Benny Baumann <[email protected]> Signed-off-by: Kang-Che Sung <[email protected]>

CI: Add backtrace screen and demangling build test

c4be1a9

For Linux and FreeBSD build jobs.

Add generic/Demangle.c

423b8d2

Signed-off-by: Kang-Che Sung <[email protected]>

Add backtrace screen

86faa3d

Co-authored-by: Benny Baumann <[email protected]>

Explorer09 force-pushed the configure-enable-backtrace branch from dd9cf40 to 86faa3d Compare December 2, 2025 17:20

MrCirdo reviewed Dec 2, 2025

View reviewed changes

Explorer09 commented Dec 3, 2025

View reviewed changes

Uh oh!

Backtrace screen (improvements over #1270) #1821

Are you sure you want to change the base?

Backtrace screen (improvements over #1270) #1821

Conversation

Explorer09 commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenBE commented Nov 16, 2025

Uh oh!

Explorer09 commented Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenBE commented Nov 16, 2025

Uh oh!

Explorer09 commented Nov 16, 2025

Uh oh!

BenBE commented Nov 16, 2025

Uh oh!

Explorer09 commented Nov 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrCirdo commented Nov 23, 2025

Uh oh!

Explorer09 commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenBE commented Nov 24, 2025

Uh oh!

Explorer09 commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrCirdo Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrCirdo commented Dec 2, 2025

Uh oh!

Explorer09 commented Dec 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Explorer09 commented Nov 15, 2025 •

edited

Loading

Explorer09 commented Nov 16, 2025 •

edited

Loading

Explorer09 commented Nov 23, 2025 •

edited

Loading

Explorer09 commented Nov 24, 2025 •

edited

Loading

MrCirdo Dec 2, 2025 •

edited

Loading