zig icon indicating copy to clipboard operation
zig copied to clipboard

std.debug: fix some corner cases

Open rootbeer opened this issue 6 months ago • 8 comments

Add infinite loop detection to the std.debug backtraces. Make the backtrace and stacktrace code more robust on corner-case architectures.

Expand the "unwind.zig" test case to exercise std.debug.dumpCurrentStackTrace(). And trigger a signal handler so the test can exercise std.debug.dumpStackTraceFromBase() and std.debug.StackIterator.initWithContext() using a kernel-constructed context.

This is preparation for moving std.debug away from getContext() (#23801).

rootbeer avatar May 19 '25 04:05 rootbeer

This is ready for a review. I think the actual fixes are all straightforward, but the test is generating a lot of stderr spew (both from the dump-stack-trace functions being tested and my verbose std.debug.print logging. This all shows up in "passing" test output (e.g., https://github.com/ziglang/zig/actions/runs/15124641832/job/42514310709?pr=23927#step:3:2243). Is there something I can do in the build.zig to hide the stderr output when the test passes? Or should I remove my print statements? But, the dump-stack-trace functions are doing most of the logging (and I suspect its generally confusing to see stack traces in passing tests). Should I disable those? (The StackIterator testing covers the meat of the code, but it does seem like the complete routines should get some testing ...)

rootbeer avatar May 20 '25 15:05 rootbeer

Is there something I can do in the build.zig to hide the stderr output when the test passes?

You can capture the output from the build script, e.g. by adding an "expected output" check. See for example #23892.

alexrp avatar May 20 '25 17:05 alexrp

@alexrp Thanks again for the reviews! One more question before I push a new version up: Should I make changes anywhere to get this test to compile/run against targets other than the default on CI?

rootbeer avatar May 20 '25 18:05 rootbeer

I guess you could just change the test's build.zig to ignore the standard target option and instead build & run for a predefined set of targets? test/llvm_targets.zig is a good resource for a list of targets that are actually relevant/real (although some of them we obviously don't fully support yet).

alexrp avatar May 20 '25 18:05 alexrp

You can capture the output from the build script, e.g. by adding an "expected output" check. See for example https://github.com/ziglang/zig/pull/23892.

Excellent! Done.

I guess you could just change the test's build.zig to ignore the standard target option and instead build & run for a predefined set of targets? test/llvm_targets.zig is a good resource for a list of targets that are actually relevant/real (although some of them we obviously don't fully support yet).

I'm going to postpone this for now. I don't want to disable the native testing, and don't really want to test a bunch of non-standard targets redundantly. I'll play around with this for future changes.

Two unrequested changes: (1) I got suspicious that the Darwin-aarch64 alignment hack-around in the test's signal handler was influencing other platforms, so I re-worked the code to more clearly do the hackery only when necessary.

rootbeer avatar May 20 '25 23:05 rootbeer

Two unrequested changes: (1) I got suspicious that the Darwin-aarch64 alignment hack-around in the test's signal handler was influencing other platforms, so I re-worked the code to more clearly do the hackery only when necessary.

What's (2)? :eyes: Also I'm not entirely sure which change you're referring to.

alexrp avatar May 21 '25 10:05 alexrp

What's (2)? 👀 Also I'm not entirely sure which change you're referring to.

Oh lol ... I was planning on changing the verification steps in the test a bit, but undid that at the last moment. Didn't quite fix up the message here well enough.

So the only thing in this latest push that needs a quick review is the aarch64-darwin ctx-fixup code in the test's signal handler. (Though wait until the tests pass, of course ...)

rootbeer avatar May 21 '25 16:05 rootbeer

This is ready for a (hopefully final) review now. Happy to make any other fixes or changes, though.

rootbeer avatar May 22 '25 16:05 rootbeer

Have you tried experimenting with this directive? https://sourceware.org/binutils/docs/as/CFI-directives.html#g_t_002ecfi_005fsignal_005fframe

alexrp avatar Aug 15 '25 06:08 alexrp

Also this definitely needs a rebase after #24960.

alexrp avatar Aug 24 '25 17:08 alexrp

I'll come back with a different set of patches after the current work in std.debug slows down (if any of these fixes are still necessary).

rootbeer avatar Sep 04 '25 04:09 rootbeer