[lit] Read command stdout/stderr as text on Windows

[mstorsjo]

Summary

This takes care of normalizing newlines back to single LF instead
of CRLF.

This on itself breaks on a couple tests that accidentally seem to
be writing binary data to stdout; make sure those cases are piped
to /dev/null instead of actually written to a terminal.

This is an alternative to half of the fixes in D120546.

[rnk]

lgtm

This will probably have the side effect of reducing the double newlines I have seen in lit test output... That could be nice.

llvm/utils/lit/lit/TestRunner.py

315

The intra-pipeline file descriptors are all *binary*, which is key to making the test suite pass.

787

docs suggest that text=True is the preferred spelling:
https://docs.python.org/3/library/subprocess.html#frequently-used-arguments

My reading of the docs is that only the files opened internally for the STDOUT / STDERR sentinel pipe values are affected by this setting, so it's safe. Anything being read by lit will be printed, and won't be important binary data.

[mstorsjo]

llvm/utils/lit/lit/TestRunner.py

787

The new option spelling text seems to exist since Python 3.7, but our build requirement is Python 3.6, so I’d stick with the older name for now.

[mstorsjo]

Squashed in a libcxx test update, removing an XFAIL, rerunning premerge tests in that form.

[mstorsjo]

This broke a lit test in utils/lit/tests/Inputs/shtest-shell/stdout-encoding.txt. (Too bad that the premerge tests are broken due to unrelated issues, hiding such failures.)

[mstorsjo]

@rnk What do you think about the test in https://github.com/llvm/llvm-project/blob/main/llvm/utils/lit/tests/Inputs/shtest-shell/stdout-encoding.txt which gets broken by this patch? It was added in https://github.com/llvm/llvm-project/commit/27fdf8a29d1e0740c342d428fa48eda7b088ac8e / D69207. Should we just conclude that we shouldn't print non-ascii to stdout in lit tests and remove that testcase?

[rnk]

In D120623#3352640, @mstorsjo wrote:

@rnk What do you think about the test in https://github.com/llvm/llvm-project/blob/main/llvm/utils/lit/tests/Inputs/shtest-shell/stdout-encoding.txt which gets broken by this patch? It was added in https://github.com/llvm/llvm-project/commit/27fdf8a29d1e0740c342d428fa48eda7b088ac8e / D69207. Should we just conclude that we shouldn't print non-ascii to stdout in lit tests and remove that testcase?

What is the failure mode? Does lit behave reasonably when binary output is printed to stdout, or does it through an exception? We should expect developers to do this by accident from time to time, and we should adjust that test to express what we want from lit. I guess I'm saying we should keep the test and update the expectations in the outer test.

[mstorsjo]

In D120623#3352655, @rnk wrote:

In D120623#3352640, @mstorsjo wrote:

@rnk What do you think about the test in https://github.com/llvm/llvm-project/blob/main/llvm/utils/lit/tests/Inputs/shtest-shell/stdout-encoding.txt which gets broken by this patch? It was added in https://github.com/llvm/llvm-project/commit/27fdf8a29d1e0740c342d428fa48eda7b088ac8e / D69207. Should we just conclude that we shouldn't print non-ascii to stdout in lit tests and remove that testcase?

What is the failure mode? Does lit behave reasonably when binary output is printed to stdout, or does it through an exception? We should expect developers to do this by accident from time to time, and we should adjust that test to express what we want from lit. I guess I'm saying we should keep the test and update the expectations in the outer test.

The failure mode is pretty nonobvious - it will indeed come up occasionally. When that happens, we get a python exception:

UNRESOLVED: shtest-shell :: stdout-encoding.txt (1 of 1)
******************** TEST 'shtest-shell :: stdout-encoding.txt' FAILED ********************
Exception during script execution:
Traceback (most recent call last):
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/worker.py", line 76, in _execute_test_handle_errors
    result = test.config.test_format.execute(test, lit_config)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/formats/shtest.py", line 27, in execute
    return lit.TestRunner.executeShTest(test, litConfig,
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 1608, in executeShTest
    return _runShTest(test, litConfig, useExternalSh, script, tmpBase)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 1556, in _runShTest 
    res = runOnce(execdir)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 1535, in runOnce
    res = executeScriptInternal(test, litConfig, tmpBase, script, execdir)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 915, in executeScriptInternal
    exitCode, timeoutInfo = executeShCmd(cmd, shenv, results, timeout=litConfig.maxIndividualTestTime)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 161, in executeShCmd
    finalExitCode = _executeShCmd(cmd, shenv, results, timeoutHelper)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 605, in _executeShCmd
    res = _executeShCmd(cmd.lhs, shenv, results, timeoutHelper)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 593, in _executeShCmd
    return _executeShCmd(cmd.rhs, shenv, results, timeoutHelper)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 816, in _executeShCmd
    procData[-1] = procs[-1].communicate()
  File "/usr/lib/python3.8/subprocess.py", line 1028, in communicate
    stdout, stderr = self._communicate(input, endtime, timeout)
  File "/usr/lib/python3.8/subprocess.py", line 1906, in _communicate
    stdout = self._translate_newlines(stdout, 
  File "/usr/lib/python3.8/subprocess.py", line 905, in _translate_newlines
    data = data.decode(encoding, errors)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 14: invalid start byte
     

********************
******************** 
Unresolved Tests (1):
  shtest-shell :: stdout-encoding.txt

[mstorsjo]

Update the lit tests references

[mstorsjo]

In D120623#3352689, @mstorsjo wrote:

What is the failure mode? Does lit behave reasonably when binary output is printed to stdout, or does it through an exception? We should expect developers to do this by accident from time to time, and we should adjust that test to express what we want from lit. I guess I'm saying we should keep the test and update the expectations in the outer test.

The failure mode is pretty nonobvious - it will indeed come up occasionally. When that happens, we get a python exception:

UNRESOLVED: shtest-shell :: stdout-encoding.txt (1 of 1)
******************** TEST 'shtest-shell :: stdout-encoding.txt' FAILED ********************
Exception during script execution:
Traceback (most recent call last):
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/worker.py", line 76, in _execute_test_handle_errors
    result = test.config.test_format.execute(test, lit_config)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/formats/shtest.py", line 27, in execute
    return lit.TestRunner.executeShTest(test, litConfig,
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 1608, in executeShTest
    return _runShTest(test, litConfig, useExternalSh, script, tmpBase)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 1556, in _runShTest 
    res = runOnce(execdir)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 1535, in runOnce
    res = executeScriptInternal(test, litConfig, tmpBase, script, execdir)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 915, in executeScriptInternal
    exitCode, timeoutInfo = executeShCmd(cmd, shenv, results, timeout=litConfig.maxIndividualTestTime)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 161, in executeShCmd
    finalExitCode = _executeShCmd(cmd, shenv, results, timeoutHelper)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 605, in _executeShCmd
    res = _executeShCmd(cmd.lhs, shenv, results, timeoutHelper)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 593, in _executeShCmd
    return _executeShCmd(cmd.rhs, shenv, results, timeoutHelper)
  File "/home/martin/code/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 816, in _executeShCmd
    procData[-1] = procs[-1].communicate()
  File "/usr/lib/python3.8/subprocess.py", line 1028, in communicate
    stdout, stderr = self._communicate(input, endtime, timeout)
  File "/usr/lib/python3.8/subprocess.py", line 1906, in _communicate
    stdout = self._translate_newlines(stdout, 
  File "/usr/lib/python3.8/subprocess.py", line 905, in _translate_newlines
    data = data.decode(encoding, errors)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 14: invalid start byte
     

********************
******************** 
Unresolved Tests (1):
  shtest-shell :: stdout-encoding.txt

@rnk Do you think this failure mode is tolerable?

[rnk]

llvm/utils/lit/lit/TestRunner.py

787

Can we pass errors="replace" to get something like the previous behavior? I think it's more helpful to show something like ?f?o?o when tests fail.

[mstorsjo]

llvm/utils/lit/lit/TestRunner.py

787

Oh, yes, that seems to work nicely!

[mstorsjo]

Add the errors = 'replace' parameter to the Popen call.

libcxx/test/libcxx/selftest/dsl/dsl.sh.py

 #===----------------------------------------------------------------------===##
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 #===----------------------------------------------------------------------===##
 
-# XFAIL: LIBCXX-WINDOWS-FIXME
-
 # Note: We prepend arguments with 'x' to avoid thinking there are too few
 #       arguments in case an argument is an empty string.
 # RUN: %{python} %s x%S x%T x%{substitutions}
 
 import base64
 import copy
 import os
 import pickle

llvm/test/MC/ELF/diff2.s

-// RUN: not llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s 2> %t
+// RUN: not llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s 2> %t > /dev/null
 // RUN: FileCheck -input-file %t %s
 
 .global zed
         .data
 foo:
         .text
         nop
 bar:
         nop
 zed:
 // CHECK: expected relocatable expression
         mov zed+(bar-foo), %eax

llvm/test/MC/Mips/relocation-xfail.s

 // RUN: llvm-mc -filetype=obj -triple mips-unknown-linux -mattr=+micromips < %s \
-// RUN:     | llvm-readobj --sections --section-data \
+// RUN:     | llvm-readobj --sections --section-data - \
 // RUN:     | FileCheck -check-prefix=DATA %s
 //
 // XFAIL: *
 
 // Please merge this with relocation.s when it passes.
 
 // baz is equivalent to .text+0x8 and is recorded in the symbol table as such
 // but it refers to microMIPS code so the addend must indicate this in the LSB.

llvm/test/Other/cfg-printer-branch-weights-percent.ll

-;RUN: opt < %s -dot-cfg -cfg-weights -cfg-dot-filename-prefix=%t 2>/dev/null
+;RUN: opt < %s -dot-cfg -cfg-weights -cfg-dot-filename-prefix=%t 2>/dev/null > /dev/null
 ;RUN: FileCheck %s -input-file=%t.f.dot
 
 define void @f(i32) {
 entry:
   %check = icmp sgt i32 %0, 0
   br i1 %check, label %if, label %exit, !prof !0
 
 ; CHECK: label="0.50%"

llvm/test/Other/cfg-printer-branch-weights.ll

-;RUN: opt < %s -dot-cfg -cfg-weights -cfg-raw-weights -cfg-dot-filename-prefix=%t 2>/dev/null
+;RUN: opt < %s -dot-cfg -cfg-weights -cfg-raw-weights -cfg-dot-filename-prefix=%t 2>/dev/null > /dev/null
 ;RUN: FileCheck %s -input-file=%t.f.dot
 
 define void @f(i32) {
 entry:
   %check = icmp sgt i32 %0, 0
   br i1 %check, label %if, label %exit, !prof !0
 
 ; CHECK: label="W:7"

llvm/test/Other/cfg-printer-filter.ll

 ; RUN: rm -f %t.other.dot %t-only.other.dot
 
 ;; Both f and func are dumped because their names contain the pattern 'f' as a substring.
-; RUN: opt < %s -dot-cfg -cfg-dot-filename-prefix=%t -cfg-func-name=f 2>/dev/null
+; RUN: opt < %s -dot-cfg -cfg-dot-filename-prefix=%t -cfg-func-name=f 2>/dev/null > /dev/null
 ; RUN: FileCheck %s -input-file=%t.f.dot -check-prefix=F
 ; RUN: FileCheck %s -input-file=%t.func.dot -check-prefix=Func
 ; RUN: not test -f %t.other.dot
 
-; RUN: opt < %s -dot-cfg-only -cfg-dot-filename-prefix=%t-only -cfg-func-name=f 2>/dev/null
+; RUN: opt < %s -dot-cfg-only -cfg-dot-filename-prefix=%t-only -cfg-func-name=f 2>/dev/null > /dev/null
 ; RUN: FileCheck %s -input-file=%t-only.f.dot -check-prefix=F
 ; RUN: FileCheck %s -input-file=%t-only.func.dot -check-prefix=Func
 ; RUN: not test -f %t-only.other.dot
 
 ; F: digraph "CFG for 'f' function"
 define void @f(i32) {
 entry:
   %check = icmp sgt i32 %0, 0

llvm/test/Other/cfg_deopt_unreach.ll

 ; RUN: rm -rf %t
 ; RUN: mkdir -p %t
-; RUN: opt < %s -dot-cfg -cfg-hide-unreachable-paths -cfg-dot-filename-prefix=%t/unreach 2>/dev/null
+; RUN: opt < %s -dot-cfg -cfg-hide-unreachable-paths -cfg-dot-filename-prefix=%t/unreach 2>/dev/null > /dev/null
 ; RUN: FileCheck %s -input-file=%t/unreach.callee.dot -check-prefix=UNREACH
-; RUN: opt < %s -dot-cfg -cfg-hide-deoptimize-paths -cfg-dot-filename-prefix=%t/deopt 2>/dev/null
+; RUN: opt < %s -dot-cfg -cfg-hide-deoptimize-paths -cfg-dot-filename-prefix=%t/deopt 2>/dev/null > /dev/null
 ; RUN: FileCheck %s -input-file=%t/deopt.callee.dot -check-prefix=DEOPT
-; RUN: opt < %s -dot-cfg -cfg-dot-filename-prefix=%t/no-flags 2>/dev/null
+; RUN: opt < %s -dot-cfg -cfg-dot-filename-prefix=%t/no-flags 2>/dev/null > /dev/null
 ; RUN: FileCheck %s -input-file=%t/no-flags.callee.dot -check-prefix=NO-FLAGS
-; RUN: opt < %s -dot-cfg -cfg-hide-unreachable-paths -cfg-hide-deoptimize-paths -cfg-dot-filename-prefix=%t/both-flags 2>/dev/null
+; RUN: opt < %s -dot-cfg -cfg-hide-unreachable-paths -cfg-hide-deoptimize-paths -cfg-dot-filename-prefix=%t/both-flags 2>/dev/null > /dev/null
 ; RUN: FileCheck %s -input-file=%t/both-flags.callee.dot -check-prefix=BOTH-FLAGS
 
 declare i8 @llvm.experimental.deoptimize.i8(...)
 
 define i8 @callee(i1* %c) alwaysinline {
   %c0 = load volatile i1, i1* %c
   br i1 %c0, label %lleft, label %lright
 ; NO-FLAGS: label="{lleft:                                            \l  %v0 = call i8 (...) @llvm.experimental.deoptimize.i8(i32 1) [ \"deopt\"(i32 1)\l... ]\l  ret i8 %v0\l}"

llvm/utils/lit/lit/TestRunner.py

if stdout == subprocess.PIPE:
         stdout = StringIO()
     elif kIsWindows:
         # Reopen stdout in binary mode to avoid CRLF translation. The versions
         # of echo we are replacing on Windows all emit plain LF, and the LLVM
         # tests now depend on this.
         # When we open as binary, however, this also means that we have to write
         # 'bytes' objects to stdout instead of 'str' objects.
         encode = lit.util.to_bytes
         stdout = open(stdout.name, stdout.mode + 'b')
         opened_files.append((None, None, stdout, None))
 
     # Implement echo flags. We only support -e and -n, and not yet in
     # combination. We have to ignore unknown flags, because `echo "-D FOO"`
     # prints the dash.
     args = cmd.args[1:]
     interpret_escapes = False
     write_newline = True
for i,j in enumerate(cmd.commands):
 
         try:
             procs.append(subprocess.Popen(args, cwd=cmd_shenv.cwd,
                                           executable = executable,
                                           stdin = stdin,
                                           stdout = stdout,
                                           stderr = stderr,
                                           env = cmd_shenv.env,
-                                          close_fds = kUseCloseFDs))
+                                          close_fds = kUseCloseFDs,
+                                          universal_newlines = True,
+                                          errors = 'replace'))
             proc_not_counts.append(not_count)
             # Let the helper know about this process
             timeoutHelper.addProcess(procs[-1])
         except OSError as e:
             raise InternalShellError(j, 'Could not create process ({}) due to {}'.format(executable, e))
 
         # Immediately close stdin for any process taking stdin from us.
         if stdin == subprocess.PIPE: