[test] Align behavior of interrupts.test on different platforms

[spavloff]

The test llvm/Support/interrupts.test behaves differently on Linux and Windows. On Linux the function 'run_wrapper' runs process with stderr connected to pipe, while on Windows stderr is mapped to stderr of the running script.

When no output was made to stderr, this difference was not observable. The new version of llvm-symbolizer (https://reviews.llvm.org/D149759) complains about missing binary file, so stderr is not empty anymore and the test fails on Windows and passes on Linux.

With this change pipes are used on all operating systems.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Changes

The test llvm/Support/interrupts.test behaves differently on Linux and Windows. On Linux the function 'run_wrapper' runs process with stderr connected to pipe, while on Windows stderr is mapped to stderr of the running script.

When no output was made to stderr, this difference was not observable. The new version of llvm-symbolizer (https://reviews.llvm.org/D149759) complains about missing binary file, so stderr is not empty anymore and the test fails on Windows and passes on Linux.

With this change pipes are used on all operating systems.

---

Full diff: https://github.com/llvm/llvm-project/pull/68556.diff

1 Files Affected:

• (modified) llvm/test/Support/interrupts.test (+1-1)

diff --git a/llvm/test/Support/interrupts.test b/llvm/test/Support/interrupts.test
index 86730f5139c042d..6e5dd5ea71f9b07 100644
--- a/llvm/test/Support/interrupts.test
+++ b/llvm/test/Support/interrupts.test
@@ -30,7 +30,7 @@ def run_wrapper():
         startupinfo = subprocess.STARTUPINFO()
         startupinfo.dwFlags |= subprocess.STARTF_USESHOWWINDOW
         proc = subprocess.Popen(args,
-                                stderr=sys.stderr,
+                                stderr=subprocess.PIPE,
                                 startupinfo=startupinfo,
                                 creationflags=subprocess.CREATE_NEW_CONSOLE)
     else:

[spavloff]

Ping.

[jh7370]

I'm not convinced the original test is correct and this commit actually makes the test worse, I think:

The test is intended to fail if llvm-symbolizer produces any stderr output on a Ctrl-Break/SIGINT. The test should therefore fail if any stderr was produced at all, including the missing binary file error message. If with your change llvm-symbolizer is producing stderr output, and the Linux test is NOT failing, that seems like a bug in the test? Conversely, if the Windows one started failing, prior to your change, I think it's working as intended (albeit picking up the "wrong" stderr).

I think the test fix that is needed is twofold:

1. The Linux stderr output needs propagating up to the lit runner so that the test fails if llvm-symbolizer produces any stderr, as intended.
2. The stdin writing needs to be such that it won't cause llvm-symbolizer to produce an error (I don't have a concrete suggestion as to what that might be). If this requires a real input, it would be better to create a minimal object file at test time, rather than try to refer to a canned object elsewhere in the testsuite, to allow this test to be self-contained.

NB: I don't think we can simply check for a specific stderr output, instead of checking that there was none, because that would introduce a race condition: llvm-symbolizer might not have written the expected message before receiving the interrupt signal. It might even have written part of the message when being interrupted.

[jh7370]

P.S. apologies for the delay in responding - I've been off work for a couple of weeks and am still catching up on things.

[spavloff]

Thank you for clarification.
In the new fix stderr is propagated to calling process so we can get actual stderr of llvm-symbolizer.
When llvm-symbolizer starts complaining on missed 'foo', we could replace

# RUN: count 0 < %t.err`

with

# RUN: FileCheck --input-file=%t.err %s
# CHECK: {{.*}} error: 'foo': {{[Nn]}}o such file or directory
# CHECK-NOT: {{.+}}
```
It should be enough to make the check you intended, no?

[jh7370]

Are you sure wait is the right thing you want? Could we simplify both this and the line a couple of lines above to subprocess.run?

[spavloff]

Are you sure wait is the right thing you want? Could we simplify both this and the line a couple of lines above to subprocess.run?

Yes, we can use subprocess.run, the PR changed accordingly.

[jh7370]

Thank you for clarification. In the new fix stderr is propagated to calling process so we can get actual stderr of llvm-symbolizer. When llvm-symbolizer starts complaining on missed 'foo', we could replace

# RUN: count 0 < %t.err`

with

# RUN: FileCheck --input-file=%t.err %s
# CHECK: {{.*}} error: 'foo': {{[Nn]}}o such file or directory
# CHECK-NOT: {{.+}}

It should be enough to make the check you intended, no?

This sounds reasonable to me. I would add as a nit that you should use the appropriate %errc substitution rather than directly using the error message.

[jh7370]

LGTM, thanks for working on this.