[lit] Fix internal diff's --strip-trailing-cr and use it

[jdenny]

Summary

Using GNU diff, --strip-trailing-cr removes a \r appearing before
a \n at the end of a line. Without this patch, lit's internal diff
only removes \r if it appears as the last character. That seems
useless. This patch fixes that.

This patch also adds --strip-trailing-cr to some tests that fail on
Windows bots when D68664 is applied. Based on what I see in the bot
logs, I think the following is happening. In each test there, lit
diff is comparing a file with \r\n line endings to a file with \n
line endings. Without D68664, lit diff reads those files in text
mode, which in Windows causes \r\n to be replaced with \n.
However, with D68664, lit diff reads the files in binary mode instead
and thus reports that every line is different, just as GNU diff does
(at least under Ubuntu). Adding --strip-trailing-cr to those tests
restores the previous behavior while permitting the behavior of lit
diff to be more like GNU diff.

[rnk]

I looked into these tests, and it seems that they would've failed if it were not for Python's universal newline translator thing. When I diff the files in question with gnu diff from git bash, they appear to be different without -w or --strip-trailing-cr. So, your fix makes lit's diff more like gnu diff, and fixes the tests to work in that mode.

The only downside is that this is one more way for tests to pass on Linux but fail on Windows out of the box. However, if we want to address that, I think we should fix it by adding a lit substitution for "\bdiff\b " to add --strip-trailing-cr on Windows.

lgtm

[jdenny]

In D68839#1706504, @rnk wrote:

I looked into these tests, and it seems that they would've failed if it were not for Python's universal newline translator thing. When I diff the files in question with gnu diff from git bash, they appear to be different without -w or --strip-trailing-cr. So, your fix makes lit's diff more like gnu diff, and fixes the tests to work in that mode.

Thanks for verifying. I only had access to logs, so I was doing some guesswork.

The only downside is that this is one more way for tests to pass on Linux but fail on Windows out of the box.

Yes, I debated whether the old behavior was desirable, but ultimately I concluded that consistency with external diffs is more important to avoid surprising behavior.

However, if we want to address that, I think we should fix it by adding a lit substitution for "\bdiff\b " to add --strip-trailing-cr on Windows.

Agreed.

[jdenny]

• Added --strip-trailing-cr to another test that needs it.
• Removed some accidental FileCheck options.
• Rebased onto master so it doesn't depend on D66574 anymore. Thus, it modifies TestRunner.py instead of diff.py.

Another review shouldn't be necessary. I plan to push tomorrow.

[jdenny]

Rebased onto D68664, which I previously forgot this patch requires. Otherwise, when using Windows or when using Python 3, a lit test introduced here fails because it expects \r\n and \n to be different while, when using Windows or Python 3, \r\n is read as \n in text mode. D68664 reads in binary mode and thus prevents that problem.

Adjusted the commit log not to confuse Python's universal newlines support (the default in Python 3 but not 2), with the \r\n -> \n behavior of text mode under even Python 2 on Windows.

llvm/test/MC/ARM/preserve-comments-arm.s

   @RUN: llvm-mc -preserve-comments -n -triple arm-eabi < %s > %t
   @RUN: sed 's/#[C]omment/@Comment/g' %s > %t2
-  @RUN: diff %t %t2
+  @RUN: diff --strip-trailing-cr %t %t2
   .text
 
   mov r0, r0
 foo:  #Comment here
   mov r0, r0  @ EOL comment
   .ident  ""
 

llvm/test/MC/AsmParser/preserve-comments.s

   #RUN: llvm-mc -preserve-comments -n -triple i386-linux-gnu < %s > %t
-  #RUN: diff %s %t
+  #RUN: diff --strip-trailing-cr %s %t
   .text
 
 foo:  #Comment here
   #comment here
   nop
   #if DIRECTIVE COMMENT
   ## WHOLE LINE COMMENT
   cmpl  $196, %eax  ## EOL COMMENT
   #endif
   .ident  "clang version 3.9.0"
   .section  ".note.GNU-stack","",@progbits

llvm/test/tools/llvm-cxxmap/remap.test

 RUN: llvm-cxxmap %S/Inputs/before.sym %S/Inputs/after.sym -r %S/Inputs/remap.map -o %t.output -Wambiguous -Wincomplete 2>&1 | FileCheck %s --allow-empty
-RUN: diff %S/Inputs/expected %t.output
+RUN: diff --strip-trailing-cr %S/Inputs/expected %t.output
 
 CHECK-NOT: warning
 CHECK-NOT: error

llvm/test/tools/llvm-profdata/profile-symbol-list.test

 ; RUN: llvm-profdata merge -sample -extbinary -prof-sym-list=%S/Inputs/profile-symbol-list-1.text %S/Inputs/sample-profile.proftext -o %t.1.output
 ; RUN: llvm-profdata merge -sample -extbinary -prof-sym-list=%S/Inputs/profile-symbol-list-2.text %S/Inputs/sample-profile.proftext -o %t.2.output
 ; RUN: llvm-profdata merge -sample -extbinary %t.1.output %t.2.output -o %t.3.output
 ; RUN: llvm-profdata show -sample -show-prof-sym-list %t.3.output > %t.4.output
-; RUN: diff %S/Inputs/profile-symbol-list.expected %t.4.output
+; RUN: diff --strip-trailing-cr %S/Inputs/profile-symbol-list.expected %t.4.output

llvm/test/tools/llvm-profdata/roundtrip.test

 RUN: llvm-profdata merge -o %t.0.profdata %S/Inputs/IR_profile.proftext
 RUN: llvm-profdata show -o %t.0.proftext -all-functions -text %t.0.profdata 
-RUN: diff %t.0.proftext %S/Inputs/IR_profile.proftext
+RUN: diff --strip-trailing-cr %t.0.proftext %S/Inputs/IR_profile.proftext
 RUN: llvm-profdata merge -o %t.1.profdata %t.0.proftext
 RUN: llvm-profdata show -o %t.1.proftext -all-functions -text %t.1.profdata 
-RUN: diff %t.1.proftext %S/Inputs/IR_profile.proftext
+RUN: diff --strip-trailing-cr %t.1.proftext %S/Inputs/IR_profile.proftext
 RUN: llvm-profdata merge --sample --binary -output=%t.2.profdata %S/Inputs/sample-profile.proftext
 RUN: llvm-profdata merge --sample --text -output=%t.2.proftext %t.2.profdata
-RUN: diff %t.2.proftext %S/Inputs/sample-profile.proftext
+RUN: diff --strip-trailing-cr %t.2.proftext %S/Inputs/sample-profile.proftext
 # Round trip from text --> extbinary --> text
 RUN: llvm-profdata merge --sample --extbinary -output=%t.3.profdata %S/Inputs/sample-profile.proftext
 RUN: llvm-profdata merge --sample --text -output=%t.3.proftext %t.3.profdata
-RUN: diff %t.3.proftext %S/Inputs/sample-profile.proftext
+RUN: diff --strip-trailing-cr %t.3.proftext %S/Inputs/sample-profile.proftext
 # Round trip from text --> binary --> extbinary --> text
 RUN: llvm-profdata merge --sample --binary -output=%t.4.profdata %S/Inputs/sample-profile.proftext
 RUN: llvm-profdata merge --sample --extbinary -output=%t.5.profdata %t.4.profdata
 RUN: llvm-profdata merge --sample --text -output=%t.4.proftext %t.5.profdata
-RUN: diff %t.4.proftext %S/Inputs/sample-profile.proftext
+RUN: diff --strip-trailing-cr %t.4.proftext %S/Inputs/sample-profile.proftext

llvm/test/tools/llvm-profdata/sample-remap.test

 ; RUN: llvm-profdata merge -sample -text %S/Inputs/sample-remap.proftext -r %S/Inputs/sample-remap.remap -o %t.output
-; RUN: diff %S/Inputs/sample-remap.expected %t.output
+; RUN: diff --strip-trailing-cr %S/Inputs/sample-remap.expected %t.output

llvm/utils/lit/lit/TestRunner.py

def compareTwoTextFiles(filepaths, encoding):
                     filelines.append(f.readlines())
 
         exitCode = 0
         def compose2(f, g):
             return lambda x: f(g(x))
 
         f = lambda x: x
         if strip_trailing_cr:
-            f = compose2(lambda line: line.rstrip('\r'), f)
+            f = compose2(lambda line: line.replace('\r\n', '\n'), f)
         if ignore_all_space or ignore_space_change:
             ignoreSpace = lambda line, separator: separator.join(line.split())
             ignoreAllSpaceOrSpaceChange = functools.partial(ignoreSpace, separator='' if ignore_all_space else ' ')
             f = compose2(ignoreAllSpaceOrSpaceChange, f)
 
         for idx, lines in enumerate(filelines):
             filelines[idx]= [f(line) for line in lines]
 

llvm/utils/lit/tests/Inputs/shtest-shell/diff-in.dos

+In this file, the
+sequence "\r\n"
+terminates lines.

llvm/utils/lit/tests/Inputs/shtest-shell/diff-in.unix

+In this file, the
+sequence "\n"
+terminates lines.

llvm/utils/lit/tests/Inputs/shtest-shell/diff-strip-trailing-cr.txt

+# Check behavior of --strip-trailing-cr.
+
+# RUN: diff -u diff-in.dos diff-in.unix && false || true
+# RUN: diff -u diff-in.unix diff-in.dos && false || true
+
+# RUN: diff -u --strip-trailing-cr diff-in.dos diff-in.unix && false || true
+# RUN: diff -u --strip-trailing-cr diff-in.unix diff-in.dos && false || true
+
+# Fail so lit will print output.
+# RUN: false

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 (31)
 # 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: *** 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: 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: # command output:
+# CHECK: @@
+# CHECK-NEXT: -In this file, the
+# CHECK-NEXT: -sequence "\r\n"
+# CHECK-NEXT: -terminates lines.
+# CHECK-NEXT: +In this file, the
+# CHECK-NEXT: +sequence "\n"
+# CHECK-NEXT: +terminates lines.
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "diff" "-u" "diff-in.unix" "diff-in.dos"
+# CHECK: # command output:
+# CHECK: @@
+# CHECK-NEXT: -In this file, the
+# CHECK-NEXT: -sequence "\n"
+# CHECK-NEXT: -terminates lines.
+# CHECK-NEXT: +In this file, the
+# CHECK-NEXT: +sequence "\r\n"
+# CHECK-NEXT: +terminates lines.
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "diff" "-u" "--strip-trailing-cr" "diff-in.dos" "diff-in.unix"
+# CHECK: # command output:
+# CHECK: @@
+# CHECK-NEXT:  In this file, the
+# CHECK-NEXT: -sequence "\r\n"
+# CHECK-NEXT: +sequence "\n"
+# CHECK-NEXT:  terminates lines.
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "diff" "-u" "--strip-trailing-cr" "diff-in.unix" "diff-in.dos"
+# CHECK: # command output:
+# CHECK: @@
+# CHECK-NEXT:  In this file, the
+# CHECK-NEXT: -sequence "\n"
+# CHECK-NEXT: +sequence "\r\n"
+# CHECK-NEXT:  terminates lines.
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "false"
+
+# CHECK: ***
+
+
 # CHECK: FAIL: shtest-shell :: diff-unified-error-0.txt
 # CHECK: *** TEST 'shtest-shell :: diff-unified-error-0.txt' FAILED ***
 # CHECK: $ "diff" "-U" "30.1" "{{[^"]*}}" "{{[^"]*}}"
 # CHECK: # command stderr:
 # CHECK: Error: invalid '-U' argument: 30.1
 # CHECK: error: command failed with exit status: 127
 # CHECK: ***
 

 # 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 (31)