From 8137f609a06bff7d7a2f5a988ee8c737ba399b19 Mon Sep 17 00:00:00 2001 From: tapplencourt Date: Tue, 23 Jun 2026 22:26:16 +0000 Subject: [PATCH] Make iprof a transparent stdio wrapper iprof ran the user binary via IO.popen, which connects the child's stdout to a pipe. The child's libc then sees a non-tty and switches to full block-buffering, so its output only appeared once it exited (e.g. all of `iprof -m full ./a.out` arrived in one block instead of streaming). Let the child inherit iprof's own stdout/stderr directly instead, so it sees exactly the descriptors iprof was given and picks the same buffering it would without iprof: line-buffered (streaming) on a terminal, block-buffered on a pipe/file. This also keeps stdout and stderr on their own streams. Add an integration test driving iprof under a PTY (via `script`) to check streaming with a real libc-buffered binary, and one checking stdout/stderr stay separate. The previous test used `bash echo`, which is not libc-buffered and so never exercised the bug. Co-Authored-By: Claude Opus 4.8 --- integration_tests/buffering_helper.c | 20 ++++++++++++++++ integration_tests/parallel_execution.bats | 28 +++++++++++++++++------ xprof/xprof.rb.in | 12 +++++----- 3 files changed, 47 insertions(+), 13 deletions(-) create mode 100644 integration_tests/buffering_helper.c diff --git a/integration_tests/buffering_helper.c b/integration_tests/buffering_helper.c new file mode 100644 index 00000000..3327c66f --- /dev/null +++ b/integration_tests/buffering_helper.c @@ -0,0 +1,20 @@ +/* + * Helper for the iprof stdout/stderr streaming tests. + * + * It uses plain stdio (printf/fprintf) with NO explicit fflush, so the C + * library decides the buffering policy from whether the fd is a tty: + * - tty -> line-buffered -> each line is emitted as it is printed + * - pipe/file -> block-buffered -> everything is flushed only at exit + */ +#include +#include + +#define SLEEP_SECONDS 4 + +int main(void) { + printf("THAPI_STREAM_FIRST\n"); + fprintf(stderr, "THAPI_STREAM_STDERR\n"); + sleep(SLEEP_SECONDS); + printf("THAPI_STREAM_SECOND\n"); + return 0; +} diff --git a/integration_tests/parallel_execution.bats b/integration_tests/parallel_execution.bats index 35d413f6..24ae395c 100644 --- a/integration_tests/parallel_execution.bats +++ b/integration_tests/parallel_execution.bats @@ -59,24 +59,38 @@ launch_mpi() { } @test "launch_usr_bin_streams_child_stdout" { - # Verify iprof streams the user binary's stdout line-by-line rather than - # buffering it. The child sleeps 4s between two prints, so under proper - # streaming the two lines reach us ~4s apart; under buffering they arrive - # together at child exit. We assert the gap is at least 2s. + # If iprof's input is not buffered, iprof should not buffer it. We use `script` + # to give iprof a tty (an unbuffered pipe); the helper sleeps 4s between two + # prints, so under streaming the two lines reach us ~4s apart. `script` runs the + # child under a PTY, whose line discipline appends a CR to each '\n', hence the + # trailing `*` in the patterns below. + helper="${BATS_TEST_TMPDIR}/buffering_helper" + gcc "${BATS_TEST_DIRNAME}/buffering_helper.c" -o "${helper}" + ts_first="" ts_second="" while IFS= read -r line; do case "$line" in - THAPI_STREAM_FIRST) ts_first=$(date +%s%N) ;; - THAPI_STREAM_SECOND) + THAPI_STREAM_FIRST*) ts_first=$(date +%s%N) ;; + THAPI_STREAM_SECOND*) ts_second=$(date +%s%N) break ;; esac - done < <(iprof -- bash -c 'echo THAPI_STREAM_FIRST; sleep 4; echo THAPI_STREAM_SECOND' 2>/dev/null) + done < <(script -qec "iprof -- '${helper}'" /dev/null) [ -n "$ts_first" ] [ -n "$ts_second" ] delta_ms=$(((ts_second - ts_first) / 1000000)) echo "delta_ms=$delta_ms" [ "$delta_ms" -gt 2000 ] } + +@test "launch_usr_bin_keeps_stderr_separate" { + # The user binary's stdout and stderr must stay on their respective streams. + helper="${BATS_TEST_TMPDIR}/buffering_helper" + gcc "${BATS_TEST_DIRNAME}/buffering_helper.c" -o "${helper}" + + run --separate-stderr iprof --no-analysis -- "${helper}" + [[ "$output" =~ $'THAPI_STREAM_FIRST\nTHAPI_STREAM_SECOND' ]] + [[ "$stderr" =~ THAPI_STREAM_STDERR ]] +} diff --git a/xprof/xprof.rb.in b/xprof/xprof.rb.in index 24907c63..95af63b0 100755 --- a/xprof/xprof.rb.in +++ b/xprof/xprof.rb.in @@ -26,7 +26,6 @@ require 'open3' require 'fileutils' require 'etc' require 'optparse_thapi' -require 'pty' require 'digest/md5' require 'socket' require 'io/nonblock' @@ -98,17 +97,18 @@ end def launch_usr_bin(env, cmd) LOGGER.info { "Launch_usr_bin #{cmd}" } - # https://docs.ruby-lang.org/en/master/IO.html#method-i-sync - $stdout.sync = true - IO.popen(env, cmd) do |io| - io.each { |line| print line } + # Let the user binary inherit our own stdout/stderr directly. Iprof is a + # transparent wrapper. + begin + pid = spawn(env, *cmd, out: $stdout, err: $stderr) + _, status = Process.wait2(pid) rescue Interrupt # Just continue as usual, do not crash xprof rescue Errno::ENOENT warn("#{__FILE__}: Can't find executable #{cmd.first}") raise Errno::ENOENT end - XprofExitCode.update($?, cmd.join(' ')) + XprofExitCode.update(status, cmd.join(' ')) if status end def exec_store_in_file(cmd, output_file: nil)