update-test-checks: safely handle tests with #if's

[nhaehnle]

Summary

There is at least one Clang test (clang/test/CodeGen/arm_acle.c) which
has functions guarded by #if's that cause those functions to be compiled
only for a subset of RUN lines.

This results in a case where one RUN line has a body for the function
and another doesn't. Treat this case as a conflict for any prefixes that
the two RUN lines have in common.

This change exposed a bug where functions with '$' in the name weren't
properly recognized in ARM assembly (despite there being a test case
that was supposed to catch the problem!). This bug is fixed as well.

[arichardson]

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/arm_function_name.ll

1–2

Remove this TODO?

[nhaehnle]

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/arm_function_name.ll

1–2

Ok, will do before I commit.

[mizvekov]

This seems to have introduced a test which always fails on windows:

$ ":" "RUN: at line 4"
$ "cp" "clang\test\utils\update_cc_test_checks/Inputs/ifdef.c" "build\llvm\tools\clang\test\utils\update_cc_test_checks\Output\ifdef.test.tmp.c"
$ "C:/Program Files (x86)/Microsoft Visual Studio/Shared/Python39_64/python.exe" "llvm\utils\update_cc_test_checks.py" "--clang" "build\llvm\RelWithDebInfo\bin\clang" "--opt" "build\llvm\RelWithDebInfo\bin\opt" "build\llvm\tools\clang\test\utils\update_cc_test_checks\Output\ifdef.test.tmp.c"
$ ":" "RUN: at line 5"
$ "diff" "-u" "clang\test\utils\update_cc_test_checks/Inputs/ifdef.c.expected" "build\llvm\tools\clang\test\utils\update_cc_test_checks\Output\ifdef.test.tmp.c"
# command output:
--- clang\test\utils\update_cc_test_checks/Inputs/ifdef.c.expected
+++ build\llvm\tools\clang\test\utils\update_cc_test_checks\Output\ifdef.test.tmp.c
@@ -1,21 +1,21 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
-// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK %s
-// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s -DFOO | FileCheck -check-prefixes=CHECK,FOO %s
-
-#ifdef FOO
-// FOO-LABEL: @foo(
-// FOO-NEXT:  entry:
-// FOO-NEXT:    ret i32 1
-//
-int foo() {
-  return 1;
-}
-#endif
-
-// CHECK-LABEL: @bar(
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:    ret i32 2
-//
-int bar() {
-  return 2;
-}
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s -DFOO | FileCheck -check-prefixes=CHECK,FOO %s
+
+#ifdef FOO
+// FOO-LABEL: @foo(
+// FOO-NEXT:  entry:
+// FOO-NEXT:    ret i32 1
+//
+int foo() {
+  return 1;
+}
+#endif
+
+// CHECK-LABEL: @bar(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    ret i32 2
+//
+int bar() {
+  return 2;
+}

error: command failed with exit status: 1

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
********************
Failed Tests (1):
  Clang :: utils/update_cc_test_checks/ifdef.test


Testing Time: 0.28s
  Failed: 1

[nhaehnle]

In D130089#3666076, @mizvekov wrote:

This seems to have introduced a test which always fails on windows:

$ ":" "RUN: at line 4"
$ "cp" "clang\test\utils\update_cc_test_checks/Inputs/ifdef.c" "build\llvm\tools\clang\test\utils\update_cc_test_checks\Output\ifdef.test.tmp.c"
$ "C:/Program Files (x86)/Microsoft Visual Studio/Shared/Python39_64/python.exe" "llvm\utils\update_cc_test_checks.py" "--clang" "build\llvm\RelWithDebInfo\bin\clang" "--opt" "build\llvm\RelWithDebInfo\bin\opt" "build\llvm\tools\clang\test\utils\update_cc_test_checks\Output\ifdef.test.tmp.c"
$ ":" "RUN: at line 5"
$ "diff" "-u" "clang\test\utils\update_cc_test_checks/Inputs/ifdef.c.expected" "build\llvm\tools\clang\test\utils\update_cc_test_checks\Output\ifdef.test.tmp.c"
# command output:
--- clang\test\utils\update_cc_test_checks/Inputs/ifdef.c.expected
+++ build\llvm\tools\clang\test\utils\update_cc_test_checks\Output\ifdef.test.tmp.c
@@ -1,21 +1,21 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
-// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK %s
-// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s -DFOO | FileCheck -check-prefixes=CHECK,FOO %s
-
-#ifdef FOO
-// FOO-LABEL: @foo(
-// FOO-NEXT:  entry:
-// FOO-NEXT:    ret i32 1
-//
-int foo() {
-  return 1;
-}
-#endif
-
-// CHECK-LABEL: @bar(
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:    ret i32 2
-//
-int bar() {
-  return 2;
-}
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s -DFOO | FileCheck -check-prefixes=CHECK,FOO %s
+
+#ifdef FOO
+// FOO-LABEL: @foo(
+// FOO-NEXT:  entry:
+// FOO-NEXT:    ret i32 1
+//
+int foo() {
+  return 1;
+}
+#endif
+
+// CHECK-LABEL: @bar(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    ret i32 2
+//
+int bar() {
+  return 2;
+}

error: command failed with exit status: 1

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
********************
Failed Tests (1):
  Clang :: utils/update_cc_test_checks/ifdef.test


Testing Time: 0.28s
  Failed: 1

Aren't the before and after lines of the diff identical? Is this a Windows new-lines issue?

[mizvekov]

In D130089#3668022, @nhaehnle wrote:

Aren't the before and after lines of the diff identical? Is this a Windows new-lines issue?

Yeah it looks like it from a glance, but I haven't had time to take a harder look at this yet.

[nhaehnle]

I've been staring at ifdef.test vs. basic-cplusplus.test for a while now and don't see any relevant difference, including when I look at the line endings in a hex editor (on Linux). So I guess there is a difference that appears only on Windows somehow? Unfortunately, I simply have no idea at the moment where it could come from.

[mizvekov]

In D130089#3668061, @nhaehnle wrote:

I've been staring at ifdef.test vs. basic-cplusplus.test for a while now and don't see any relevant difference, including when I look at the line endings in a hex editor (on Linux). So I guess there is a difference that appears only on Windows somehow? Unfortunately, I simply have no idea at the moment where it could come from.

Yeah it could be that there is some flag we should be passing to diff, or some other thing that we normally do on llvm to take care of this issue, so that diff ignores the line ending difference.
I haven't had time to look at what is the standard practice we have for that yet.

[nhaehnle]

My version of diff has a --strip-trailing-cr flag. But I don't really see that being used in many places at all.

[mizvekov]

In D130089#3668111, @nhaehnle wrote:

My version of diff has a --strip-trailing-cr flag. But I don't really see that being used in many places at all.

What I see is that the sources are checked out with the expected windows (\r\n) line endings, but the file we generate (build\llvm\tools\clang\test\utils\update_cc_test_checks\Output\ifdef.test.tmp.c) has unix (\n) line endings.
I am not sure yet what we would usually do here, but it makes more sense to generate the file with line endings expected for the native platform.

[mizvekov]

I see what is going on here. The problem is not in this patch, but there is a problem in update_cc_test_checks.py.

I had all the other files in clang/test/utils/update_cc_test_checks still checked out as unix line endings from before I had updated the config, and that is why they were not failing.

Fixing update_cc_test_checks.py and restoring those other files makes every test pass.
Here is my diff:

diff --git a/llvm/utils/update_cc_test_checks.py b/llvm/utils/update_cc_test_checks.py
index b9e91f19461b..0e755d100760 100755
--- a/llvm/utils/update_cc_test_checks.py
+++ b/llvm/utils/update_cc_test_checks.py
@@ -414,7 +414,7 @@ def main():
                                output_lines, global_vars_seen_dict, True, False)
     common.debug('Writing %d lines to %s...' % (len(output_lines), ti.path))
     with open(ti.path, 'wb') as f:
-      f.writelines(['{}\n'.format(l).encode('utf-8') for l in output_lines])
+      f.writelines(['{}{}'.format(l, os.linesep).encode('utf-8') for l in output_lines])

   return 0

I will submit a MR with this patch later today.

clang/test/utils/update_cc_test_checks/Inputs/ifdef.c

+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s -DFOO | FileCheck -check-prefixes=CHECK,FOO %s
+
+#ifdef FOO
+int foo() {
+  return 1;
+}
+#endif
+
+int bar() {
+  return 2;
+}

clang/test/utils/update_cc_test_checks/Inputs/ifdef.c.expected

+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s -DFOO | FileCheck -check-prefixes=CHECK,FOO %s
+
+#ifdef FOO
+// FOO-LABEL: @foo(
+// FOO-NEXT:  entry:
+// FOO-NEXT:    ret i32 1
+//
+int foo() {
+  return 1;
+}
+#endif
+
+// CHECK-LABEL: @bar(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    ret i32 2
+//
+int bar() {
+  return 2;
+}

clang/test/utils/update_cc_test_checks/ifdef.test

+## Test that functions that are only compiled in a subset of RUN lines are
+## handled correctly
+
+# RUN: cp %S/Inputs/ifdef.c %t.c && %update_cc_test_checks %t.c
+# RUN: diff -u %S/Inputs/ifdef.c.expected %t.c
+## Check that re-running update_cc_test_checks doesn't change the output
+# RUN: %update_cc_test_checks %t.c
+# RUN: diff -u %S/Inputs/ifdef.c.expected %t.c

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/arm_function_name.ll

 ; Check that we accept functions with '$' in the name.
-; TODO: This is not handled correcly on 32bit ARM and needs to be fixed.
 
-; RUN: llc -mtriple=armv7-unknown-linux < %s | FileCheck --prefix=LINUX %s
-; RUN: llc -mtriple=armv7-apple-darwin < %s | FileCheck --prefix=DARWIN %s
-; RUN: llc -mtriple=armv7-apple-ios < %s | FileCheck --prefix=IOS %s
+; RUN: llc -mtriple=armv7-unknown-linux < %s | FileCheck %s
+; RUN: llc -mtriple=armv7-apple-darwin < %s | FileCheck %s
+; RUN: llc -mtriple=armv7-apple-ios < %s | FileCheck %s
 
 define hidden i32 @"_Z54bar$ompvariant$bar"() {
 entry:
   ret i32 2
 }

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/arm_function_name.ll.expected

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; Check that we accept functions with '$' in the name.
-; TODO: This is not handled correcly on 32bit ARM and needs to be fixed.
 
-; RUN: llc -mtriple=armv7-unknown-linux < %s | FileCheck --prefix=LINUX %s
-; RUN: llc -mtriple=armv7-apple-darwin < %s | FileCheck --prefix=DARWIN %s
-; RUN: llc -mtriple=armv7-apple-ios < %s | FileCheck --prefix=IOS %s
+; RUN: llc -mtriple=armv7-unknown-linux < %s | FileCheck %s
+; RUN: llc -mtriple=armv7-apple-darwin < %s | FileCheck %s
+; RUN: llc -mtriple=armv7-apple-ios < %s | FileCheck %s
 
 define hidden i32 @"_Z54bar$ompvariant$bar"() {
 ; CHECK-LABEL: _Z54bar$ompvariant$bar:
 ; CHECK:       @ %bb.0: @ %entry
 ; CHECK-NEXT:    mov r0, #2
 ; CHECK-NEXT:    bx lr
 entry:
   ret i32 2
 }

llvm/utils/UpdateTestChecks/asm.py

ASM_FUNCTION_X86_RE = re.compile(
     r'^_?(?P<func>[^:]+):[ \t]*#+[ \t]*(@"?(?P=func)"?| -- Begin function (?P=func))\n(?:\s*\.?Lfunc_begin[^:\n]*:\n)?'
     r'(?:\.L[^$]+\$local:\n)?'      # drop .L<func>$local:
     r'(?:[ \t]+.cfi_startproc\n|.seh_proc[^\n]+\n)?'  # drop optional cfi
     r'(?P<body>^##?[ \t]+[^:]+:.*?)\s*'
     r'^\s*(?:[^:\n]+?:\s*\n\s*\.size|\.cfi_endproc|\.globl|\.comm|\.(?:sub)?section|#+ -- End function)',
     flags=(re.M | re.S))
 
 ASM_FUNCTION_ARM_RE = re.compile(
-    r'^(?P<func>[0-9a-zA-Z_]+):\n' # f: (name of function)
+    r'^(?P<func>[0-9a-zA-Z_$]+):\n' # f: (name of function)
     r'\s+\.fnstart\n' # .fnstart
-    r'(?P<body>.*?)\n' # (body of the function)
-    r'.Lfunc_end[0-9]+:', # .Lfunc_end0: or # -- End function
+    r'(?P<body>.*?)' # (body of the function)
+    r'^.Lfunc_end[0-9]+:', # .Lfunc_end0: or # -- End function
     flags=(re.M | re.S))
 
 ASM_FUNCTION_AARCH64_RE = re.compile(
      r'^_?(?P<func>[^:]+):[ \t]*\/\/[ \t]*@"?(?P=func)"?( (Function|Tail Call))?\n'
      r'(?:[ \t]+.cfi_startproc\n)?'  # drop optional cfi noise
      r'(?P<body>.*?)\n'
      # This list is incomplete
      r'^\s*(\.Lfunc_end[0-9]+|// -- End function)',
ASM_FUNCTION_AARCH64_DARWIN_RE = re.compile(
     r'^_(?P<func>[^:]+):[ \t]*;[ \t]@"?(?P=func)"?\n'
     r'([ \t]*.cfi_startproc\n[\s]*)?'
     r'(?P<body>.*?)'
     r'([ \t]*.cfi_endproc\n[\s]*)?'
     r'^[ \t]*;[ \t]--[ \t]End[ \t]function',
     flags=(re.M | re.S))
 
 ASM_FUNCTION_ARM_DARWIN_RE = re.compile(
-    r'^[ \t]*\.globl[ \t]*_(?P<func>[^ \t])[ \t]*@[ \t]--[ \t]Begin[ \t]function[ \t]"?(?P=func)"?'
+    r'@[ \t]--[ \t]Begin[ \t]function[ \t](?P<func>[^ \t]+?)\n'
+    r'^[ \t]*\.globl[ \t]*_(?P=func)[ \t]*'
     r'(?P<directives>.*?)'
     r'^_(?P=func):\n[ \t]*'
     r'(?P<body>.*?)'
     r'^[ \t]*@[ \t]--[ \t]End[ \t]function',
     flags=(re.M | re.S ))
 
 ASM_FUNCTION_ARM_MACHO_RE = re.compile(
     r'^_(?P<func>[^:]+):[ \t]*\n'

llvm/utils/UpdateTestChecks/common.py

self._filters = list(map(lambda f: Filter(re.compile(f.pattern().strip('"'),
                              flags.filters)) if flags.filters else []
     self._scrubber_args = scrubber_args
     self._path = path
     # Strip double-quotes if input was read by UTC_ARGS
     self._replace_value_regex = list(map(lambda x: x.strip('"'), flags.replace_value_regex))
     self._func_dict = {}
     self._func_order = {}
     self._global_var_dict = {}
+    self._processed_prefixes = set()
     for tuple in run_list:
       for prefix in tuple[0]:
         self._func_dict.update({prefix:dict()})
         self._func_order.update({prefix: []})
         self._global_var_dict.update({prefix:dict()})
 
   def finish_and_get_func_dict(self):
     for prefix in self.get_failed_prefixes():
for m in function_re.finditer(raw_tool_output):
             for g in match.groups():
               func_repl = group_regex.sub(re.escape(g), func_repl, count=1)
             # Substitute function call names that match the regex with the same
             # capture groups set.
             scrubbed_body = re.sub(func_repl, '{{' + func_repl + '}}',
                                    scrubbed_body)
 
         if func in self._func_dict[prefix]:
-          if (self._func_dict[prefix][func] is None or
-              str(self._func_dict[prefix][func]) != scrubbed_body or
-              self._func_dict[prefix][func].args_and_sig != args_and_sig or
-                  self._func_dict[prefix][func].attrs != attrs):
-            if (self._func_dict[prefix][func] is not None and
-                self._func_dict[prefix][func].is_same_except_arg_names(
+              (str(self._func_dict[prefix][func]) != scrubbed_body or
+               self._func_dict[prefix][func].args_and_sig != args_and_sig or
+               self._func_dict[prefix][func].attrs != attrs)):
+            if self._func_dict[prefix][func].is_same_except_arg_names(
                 scrubbed_extra,
                 args_and_sig,
                 attrs,
-                is_backend)):
+                is_backend):
               self._func_dict[prefix][func].scrub = scrubbed_extra
               self._func_dict[prefix][func].args_and_sig = args_and_sig
-              continue
             else:
               # This means a previous RUN line produced a body for this function
               # that is different from the one produced by this current RUN line,
               # so the body can't be common accross RUN lines. We use None to
               # indicate that.
               self._func_dict[prefix][func] = None
-              continue
-
+        else:
+          if prefix not in self._processed_prefixes:
             self._func_dict[prefix][func] = function_body(
-            scrubbed_body, scrubbed_extra, args_and_sig, attrs, func_name_separator)
+                scrubbed_body, scrubbed_extra, args_and_sig, attrs,
+                func_name_separator)
             self._func_order[prefix].append(func)
+          else:
+            # An earlier RUN line used this check prefixes but didn't produce
+            # a body for this function. This happens in Clang tests that use
+            # preprocesser directives to exclude individual functions from some
+            # RUN lines.
+            self._func_dict[prefix][func] = None
+
+  def processed_prefixes(self, prefixes):
+    """
+    Mark a set of prefixes as having had at least one applicable RUN line fully
+    processed. This is used to filter out function bodies that don't have
+    outputs for all RUN lines.
+    """
+    self._processed_prefixes.update(prefixes)
 
   def get_failed_prefixes(self):
     # This returns the list of those prefixes that failed to match any function,
     # because there were conflicting bodies produced by different RUN lines, in
     # all instances of the prefix.
     for prefix in self._func_dict:
       if (self._func_dict[prefix] and
           (not [fct for fct in self._func_dict[prefix]

llvm/utils/update_analyze_test_checks.py

for prefixes, opt_args in prefix_list:
         # Split analysis outputs by "Printing analysis " declarations.
         for raw_tool_output in re.split(r'LV: Checking a loop in ', raw_tool_outputs):
           builder.process_run_line(common.LV_DEBUG_RE, common.scrub_body,
                                   raw_tool_output, prefixes, False)
       else:
         common.warn('Don\'t know how to deal with this output')
         continue
 
+      builder.processed_prefixes(prefixes)
+
     func_dict = builder.finish_and_get_func_dict()
     is_in_function = False
     is_in_function_start = False
     prefix_set = set([prefix for prefixes, _ in prefix_list for prefix in prefixes])
     common.debug('Rewriting FileCheck prefixes:', str(prefix_set), file=sys.stderr)
     output_lines = []
 
     for input_info in ti.iterlines(output_lines):

llvm/utils/update_cc_test_checks.py

with tempfile.NamedTemporaryFile() as f:
           sys.exit(1)
         extra_args[0] = args.opt
       raw_tool_output = common.invoke_tool(extra_args[0],
                                            extra_args[1:], f.name)
   if '-emit-llvm' in clang_args:
     builder.process_run_line(
             common.OPT_FUNCTION_RE, common.scrub_body, raw_tool_output,
             prefixes, False)
+    builder.processed_prefixes(prefixes)
   else:
     print('The clang command line should include -emit-llvm as asm tests '
           'are discouraged in Clang testsuite.', file=sys.stderr)
     sys.exit(1)
 
 def exec_run_line(exe):
   popen = subprocess.Popen(exe, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
   stdout, stderr = popen.communicate()

llvm/utils/update_llc_test_checks.py

for prefixes, llc_tool, llc_args, preprocess_cmd, triple_in_cmd, march_in_cmd in run_list:
                                            llc_args, ti.path, preprocess_cmd,
                                            verbose=ti.args.verbose)
       triple = triple_in_cmd or triple_in_ir
       if not triple:
         triple = common.get_triple_from_march(march_in_cmd)
 
       scrubber, function_re = output_type.get_run_handler(triple)
       builder.process_run_line(function_re, scrubber, raw_tool_output, prefixes, True)
+      builder.processed_prefixes(prefixes)
 
     func_dict = builder.finish_and_get_func_dict()
     global_vars_seen_dict = {}
 
     is_in_function = False
     is_in_function_start = False
     func_name = None
     prefix_set = set([prefix for p in run_list for prefix in p[0]])

llvm/utils/update_test_checks.py

for prefixes, opt_args, preprocess_cmd in prefix_list:
       common.debug('Extracted opt cmd: ' + opt_basename + ' ' + opt_args)
       common.debug('Extracted FileCheck prefixes: ' + str(prefixes))
 
       raw_tool_output = common.invoke_tool(ti.args.opt_binary, opt_args,
                                            ti.path, preprocess_cmd=preprocess_cmd,
                                            verbose=ti.args.verbose)
       builder.process_run_line(common.OPT_FUNCTION_RE, common.scrub_body,
               raw_tool_output, prefixes, False)
+      builder.processed_prefixes(prefixes)
 
     func_dict = builder.finish_and_get_func_dict()
     is_in_function = False
     is_in_function_start = False
     has_checked_pre_function_globals = False
     prefix_set = set([prefix for prefixes, _, _ in prefix_list for prefix in prefixes])
     common.debug('Rewriting FileCheck prefixes:', str(prefix_set))
     output_lines = []