[lit] Extend internal diff to support `-` argument

[jdenny]

Summary

When using lit's internal shell, RUN lines like the following
accidentally execute an external diff instead of lit's internal
diff:

# RUN: program | diff file -

Such cases exist now, in clang/test/Analysis for example. We are
preparing patches to ensure lit's internal diff is called in such
cases, which will then fail because lit's internal diff doesn't
recognize - as a command-line option. This patch adds support for
- to mean stdin.

[rnk]

llvm/utils/lit/lit/builtin_commands/diff.py

32–34

This function seems fragile to me. It reads each file three times:

1. without an encoding (system default?), may fail
2. with a utf-8 encoding, may fail
3. inside compareTwo*Files, reads them yet again

I think the best way to be portable between Python 2 & 3 will probably be to work in bytes as much as possible.

I think you can use this pattern to get a new file descriptor for stdin that reads bytes:

fileno = sys.stdin.fileno()
if fileno is not None:
    new_stdin = os.fdopen(os.dup(fileno), 'rb') # read in binary

Here's how I would do it:

1. read both inputs completely in binary (can't fail)
2. try decoding the entire file in a few encodings (try default, try utf-8, if that fails, stick with bytes and diff_bytes)
3. split file on u'\n' or b'\n' as appropriate, perhaps stripping trailing '\r' if present, depending on flags

Does that seem reasonable?

[jdenny]

llvm/utils/lit/lit/builtin_commands/diff.py

32–34

I don't know the motivation by the original implementation. Your plan sounds better. Sorry, but I don't have time to implement and test this right now. Hopefully soon. Thanks for the review.

[jdenny]

llvm/utils/lit/lit/builtin_commands/diff.py

32–34

I think you can use this pattern to get a new file descriptor for stdin that reads bytes:

fileno = sys.stdin.fileno()
if fileno is not None:

I don't see documentation saying that sys.stdin.fileno might return None. Is that possible?

[jdenny]

Rebased onto D68664, and extended its encoding tests for -.

[rnk]

lgtm

llvm/utils/lit/lit/builtin_commands/diff.py

32–34

I think I just copied it from stack overflow here: https://stackoverflow.com/questions/32199552/what-is-sys-stdin-fileno-in-python

I guess in this case os.dup would fail, which is a reasonable failure mode.

[jdenny]

Rebased onto the most recent D66574.

[jdenny]

Rebased onto updated D66574.

When echo appears in a pipeline, lit calls an external echo, which terminates lines with \r\n on Windows. Otherwise, lit uses its internal echo, which terminates lines with \n, just as does lit's out-of-process builtin cat. Because lit's internal diff is sensitive to this difference, after echo foo > %t.foo, do cat %t.foo | diff - %t.foo not echo foo | diff - %t.foo in lit's test suite in this patch.

llvm/utils/lit/lit/builtin_commands/diff.py

def getDirTree(path, basedir=""):
     for dirname, child_dirs, files in os.walk(os.path.join(basedir, path)):
         for child_dir in child_dirs:
             child_trees.append(getDirTree(child_dir, dirname))
         for filename in files:
             child_trees.append((filename, None))
         return path, sorted(child_trees)
 
 def compareTwoFiles(flags, filepaths):
     filelines = []
     for file in filepaths:
+        if file == "-":
+            stdin_fileno = sys.stdin.fileno()
+            with os.fdopen(os.dup(stdin_fileno), 'rb') as stdin_bin:
+                filelines.append(stdin_bin.readlines())
+        else:
             with open(file, 'rb') as file_bin:
                 filelines.append(file_bin.readlines())
 
     try:
         return compareTwoTextFiles(flags, filepaths, filelines,
                                    locale.getpreferredencoding(False))
     except UnicodeDecodeError:
         try:
             return compareTwoTextFiles(flags, filepaths, filelines, "utf-8")
         except:
def main(argv):
 
     if len(args) != 2:
         sys.stderr.write("Error: missing or extra operand\n")
         sys.exit(1)
 
     exitCode = 0
     try:
         for file in args:
-            if not os.path.isabs(file):
+            if file != "-" and not os.path.isabs(file):
                 file = os.path.realpath(os.path.join(os.getcwd(), file))
 
             if flags.recursive_diff:
+                if file == "-":
+                    sys.stderr.write("Error: cannot recursively compare '-'\n")
+                    sys.exit(1)
                 dir_trees.append(getDirTree(file))
             else:
                 filepaths.append(file)
 
         if not flags.recursive_diff:
             exitCode = compareTwoFiles(flags, filepaths)
         else:
             exitCode = compareDirTrees(flags, dir_trees)

llvm/utils/lit/tests/Inputs/shtest-shell/diff-encodings.txt

 # Check that diff falls back to binary mode if it cannot decode a file.
 
 # RUN: diff -u diff-in.bin diff-in.bin
 # RUN: diff -u diff-in.utf16 diff-in.bin && false || true
 # RUN: diff -u diff-in.utf8 diff-in.bin && false || true
 # RUN: diff -u diff-in.bin diff-in.utf8 && false || true
 
+# RUN: cat diff-in.bin | diff -u - diff-in.bin
+# RUN: cat diff-in.bin | diff -u diff-in.bin -
+# RUN: cat diff-in.bin | diff -u diff-in.utf16 - && false || true
+# RUN: cat diff-in.bin | diff -u diff-in.utf8 - && false || true
+# RUN: cat diff-in.bin | diff -u - diff-in.utf8 && false || true
+
 # Fail so lit will print output.
 # RUN: false

llvm/utils/lit/tests/Inputs/shtest-shell/diff-pipes.txt

 # RUN: echo foo > %t.foo
 # RUN: echo bar > %t.bar
 
 # Check output pipe.
 # RUN: diff %t.foo %t.foo | FileCheck -allow-empty -check-prefix=EMPTY %s
 # RUN: diff -u %t.foo %t.bar | FileCheck %s && false || true
 
+# Check input pipe.
+# RUN: echo foo | diff -u - %t.foo
+# RUN: echo foo | diff -u %t.foo -
+# RUN: echo bar | diff -u %t.foo - && false || true
+# RUN: echo bar | diff -u - %t.foo && false || true
+
+# Check output and input pipes at the same time.
+# RUN: echo foo | diff - %t.foo | FileCheck -allow-empty -check-prefix=EMPTY %s
+# RUN: echo bar | diff -u %t.foo - | FileCheck %s && false || true
+
 # Fail so lit will print output.
 # RUN: false
 
 # CHECK:      @@
 # CHECK-NEXT: -foo
 # CHECK-NEXT: +bar
 
 # EMPTY-NOT: {{.}}

llvm/utils/lit/tests/Inputs/shtest-shell/diff-r-error-7.txt

+# diff -r currently cannot handle stdin.
+# RUN: diff -r - %t

llvm/utils/lit/tests/Inputs/shtest-shell/diff-r-error-8.txt

+# diff -r currently cannot handle stdin.
+# RUN: diff -r %t -

llvm/utils/lit/tests/max-failures.py

 # Check the behavior of --max-failures option.
 #
 # RUN: not %{lit} -j 1 -v %{inputs}/max-failures > %t.out
 # RUN: not %{lit} --max-failures=1 -j 1 -v %{inputs}/max-failures >> %t.out
 # RUN: not %{lit} --max-failures=2 -j 1 -v %{inputs}/max-failures >> %t.out
 # RUN: not %{lit} --max-failures=0 -j 1 -v %{inputs}/max-failures 2>> %t.out
 # RUN: FileCheck < %t.out %s
 #
 # END.
 
-# CHECK: Failing Tests (30)
+# CHECK: Failing Tests (32)
 # CHECK: Failing Tests (1)
 # CHECK: Failing Tests (2)
 # CHECK: error: argument --max-failures: requires positive integer, but found '0'

llvm/utils/lit/tests/shtest-shell.py

 # CHECK-NEXT: {{^\-.b.a.r..}}
 # CHECK-NEXT: {{^\-.b.a.z.$}}
 # CHECK-NEXT: +foo
 # CHECK-NEXT: +bar
 # CHECK-NEXT: +baz
 # CHECK: error: command failed with exit status: 1
 # CHECK: $ "true"
 
+# CHECK: $ "cat" "diff-in.bin"
+# CHECK-NOT: error
+# CHECK: $ "diff" "-u" "-" "diff-in.bin"
+# CHECK-NOT: error
+
+# CHECK: $ "cat" "diff-in.bin"
+# CHECK-NOT: error
+# CHECK: $ "diff" "-u" "diff-in.bin" "-"
+# CHECK-NOT: error
+
+# CHECK: $ "cat" "diff-in.bin"
+# CHECK-NOT: error
+# CHECK: $ "diff" "-u" "diff-in.utf16" "-"
+# CHECK: # command output:
+# CHECK-NEXT: ---
+# CHECK-NEXT: +++
+# CHECK-NEXT: @@
+# CHECK-NEXT: {{^ .f.o.o.$}}
+# CHECK-NEXT: {{^-.b.a.r.$}}
+# CHECK-NEXT: {{^\+.b.a.r..}}
+# CHECK-NEXT: {{^ .b.a.z.$}}
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "cat" "diff-in.bin"
+# CHECK-NOT: error
+# CHECK: $ "diff" "-u" "diff-in.utf8" "-"
+# CHECK: # command output:
+# CHECK-NEXT: ---
+# CHECK-NEXT: +++
+# CHECK-NEXT: @@
+# CHECK-NEXT: -foo
+# CHECK-NEXT: -bar
+# CHECK-NEXT: -baz
+# CHECK-NEXT: {{^\+.f.o.o.$}}
+# CHECK-NEXT: {{^\+.b.a.r..}}
+# CHECK-NEXT: {{^\+.b.a.z.$}}
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "diff" "-u" "-" "diff-in.utf8"
+# CHECK: # command output:
+# CHECK-NEXT: ---
+# CHECK-NEXT: +++
+# CHECK-NEXT: @@
+# CHECK-NEXT: {{^\-.f.o.o.$}}
+# CHECK-NEXT: {{^\-.b.a.r..}}
+# CHECK-NEXT: {{^\-.b.a.z.$}}
+# CHECK-NEXT: +foo
+# CHECK-NEXT: +bar
+# CHECK-NEXT: +baz
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
 # CHECK: $ "false"
 
 # CHECK: ***
 
 
 # CHECK: FAIL: shtest-shell :: diff-error-1.txt
 # CHECK: *** TEST 'shtest-shell :: diff-error-1.txt' FAILED ***
 # CHECK: $ "diff" "-B" "temp1.txt" "temp2.txt"

 # CHECK: $ "diff" "-u" "{{[^"]*}}.foo" "{{[^"]*}}.bar"
 # CHECK: note: command had no output on stdout or stderr
 # CHECK: error: command failed with exit status: 1
 # CHECK: $ "FileCheck"
 # CHECK-NOT: note
 # CHECK-NOT: error
 # CHECK: $ "true"
 
+# CHECK: $ "echo" "foo"
+# CHECK: $ "diff" "-u" "-" "{{[^"]*}}.foo"
+# CHECK-NOT: note
+# CHECK-NOT: error
+
+# CHECK: $ "echo" "foo"
+# CHECK: $ "diff" "-u" "{{[^"]*}}.foo" "-"
+# CHECK-NOT: note
+# CHECK-NOT: error
+
+# CHECK: $ "echo" "bar"
+# CHECK: $ "diff" "-u" "{{[^"]*}}.foo" "-"
+# CHECK: # command output:
+# CHECK: @@
+# CHECK-NEXT: -foo
+# CHECK-NEXT: +bar
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "echo" "bar"
+# CHECK: $ "diff" "-u" "-" "{{[^"]*}}.foo"
+# CHECK: # command output:
+# CHECK: @@
+# CHECK-NEXT: -bar
+# CHECK-NEXT: +foo
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "echo" "foo"
+# CHECK: $ "diff" "-" "{{[^"]*}}.foo"
+# CHECK-NOT: note
+# CHECK-NOT: error
+# CHECK: $ "FileCheck"
+# CHECK-NOT: note
+# CHECK-NOT: error
+
+# CHECK: $ "echo" "bar"
+# CHECK: $ "diff" "-u" "{{[^"]*}}.foo" "-"
+# CHECK: note: command had no output on stdout or stderr
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "FileCheck"
+# CHECK-NOT: note
+# CHECK-NOT: error
+# CHECK: $ "true"
+
 # CHECK: $ "false"
 
 # CHECK: ***
 
 
 # CHECK: FAIL: shtest-shell :: diff-r-error-0.txt
 # CHECK: *** TEST 'shtest-shell :: diff-r-error-0.txt' FAILED ***
 # CHECK: $ "diff" "-r" 

 
 # CHECK: FAIL: shtest-shell :: diff-r-error-6.txt
 # CHECK: *** TEST 'shtest-shell :: diff-r-error-6.txt' FAILED ***
 # CHECK: $ "diff" "-r" 
 # CHECK: # command output:
 # CHECK: File {{.*}}dir1{{.*}}extra_file is a regular empty file while file {{.*}}dir2{{.*}}extra_file is a directory
 # CHECK: error: command failed with exit status: 1
 
+# CHECK: FAIL: shtest-shell :: diff-r-error-7.txt
+# CHECK: *** TEST 'shtest-shell :: diff-r-error-7.txt' FAILED ***
+# CHECK: $ "diff" "-r" "-" "{{[^"]*}}"
+# CHECK: # command stderr:
+# CHECK: Error: cannot recursively compare '-'
+# CHECK: error: command failed with exit status: 1
+
+# CHECK: FAIL: shtest-shell :: diff-r-error-8.txt
+# CHECK: *** TEST 'shtest-shell :: diff-r-error-8.txt' FAILED ***
+# CHECK: $ "diff" "-r" "{{[^"]*}}" "-"
+# CHECK: # command stderr:
+# CHECK: Error: cannot recursively compare '-'
+# CHECK: error: command failed with exit status: 1
+
 # CHECK: PASS: shtest-shell :: diff-r.txt
 
 
 # CHECK: FAIL: shtest-shell :: diff-strip-trailing-cr.txt
 
 # CHECK: *** TEST 'shtest-shell :: diff-strip-trailing-cr.txt' FAILED ***
 
 # CHECK: $ "diff" "-u" "diff-in.dos" "diff-in.unix"

 # CHECK: *** TEST 'shtest-shell :: rm-error-3.txt' FAILED ***
 # CHECK: Exit Code: 1
 # CHECK: ***
 
 # CHECK: PASS: shtest-shell :: rm-unicode-0.txt
 # CHECK: PASS: shtest-shell :: sequencing-0.txt
 # CHECK: XFAIL: shtest-shell :: sequencing-1.txt
 # CHECK: PASS: shtest-shell :: valid-shell.txt
-# CHECK: Failing Tests (30)
+# CHECK: Failing Tests (32)