[Utils] Add --full-function-signature to update_cc_test_checks.py to match return type as well as args

[asb]

Summary

This is an alternative, less intrusive take on D133943 (where we were unable to find consensus on how to handle updating existing in-tree tests).

Zooming right out, the basic problem is that --function-signature checks arguments but not the return type. This limitation means it's not very useful for my use case (making RISC-V ABI tests more maintainable). The value of this is huge - manually updating CHECK lines in the previous ABI tests was a massive timesink, and with this feature it is _massively_ easier.

Although it would be cleaner in a sense to update --function-signature and find agreement on transitioning in-tree tests to the new behaviour, I think adding the separate command-line flag is the best option available at this point. It's hopefully much easier to get this reviewed and landed, thus unblocking the ABI test refactorings and follow-on bug fixes that rely on it. There's also a clear migration path - in the future --function-signature could be updated to act like --full-function-signature and --full-function-signature be either removed or left as an alias. It would be slightly better to support --function-signature=full, but I couldn't find a way to accept an optional argument but only with --foo=bar syntax (rather than --foo bar, which could break existing command lines) with default argparse routines.

I'd really, really, really appreciate help on unblocking this (reviews or other feedback). Thank you in advance.

[luismarques]

Do we actually want to include the dso_local in the full signature?

[asb]

In D142373#4076198, @luismarques wrote:

Do we actually want to include the dso_local in the full signature?

That's a fair question. I could imagine it's useful to include these attributes in some test scenarios and not in others. I'm not sure it's worth adding another flag and more logic to optionally strip it.

[luismarques]

I'm probably not the best person to review this but, for what it's worth, it LGTM.

[nikic]

Please give me a day to propose another alternative... Seeing D142452 we now have three different patches that would like to change UTC output, so I really think we need that --version flag.

[asb]

In D142373#4076584, @nikic wrote:

Please give me a day to propose another alternative... Seeing D142452 we now have three different patches that would like to change UTC output, so I really think we need that --version flag.

Of course, thanks for taking a look. This incarnation of the patch of course is explicitly trying to avoid change default UTC output, at the cost of adding yet another command line option.

[nikic]

D142473 would be my proposal to fix the test churn problem going forward.

[asb]

Abandoned in favour of https://reviews.llvm.org/D144963 which rebases this logic on top of the new --version support.

clang/test/utils/update_cc_test_checks/Inputs/mangled_names.c.fullfuncsig.expected

+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --full-function-signature
+// Example input for update_cc_test_checks
+// RUN: %clang_cc1 -no-opaque-pointers -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define dso_local i64 @test
+// CHECK-SAME: (i64 noundef [[A:%.*]], i32 noundef [[B:%.*]]) #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[A_ADDR:%.*]] = alloca i64, align 8
+// CHECK-NEXT:    [[B_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store i64 [[A]], i64* [[A_ADDR]], align 8
+// CHECK-NEXT:    store i32 [[B]], i32* [[B_ADDR]], align 4
+// CHECK-NEXT:    [[TMP0:%.*]] = load i64, i64* [[A_ADDR]], align 8
+// CHECK-NEXT:    [[TMP1:%.*]] = load i32, i32* [[B_ADDR]], align 4
+// CHECK-NEXT:    [[CONV:%.*]] = sext i32 [[TMP1]] to i64
+// CHECK-NEXT:    [[ADD:%.*]] = add nsw i64 [[TMP0]], [[CONV]]
+// CHECK-NEXT:    ret i64 [[ADD]]
+//
+long test(long a, int b) {
+  return a + b;
+}
+
+// A function with a mangled name
+// CHECK-LABEL: define dso_local i64 @_Z4testlii
+// CHECK-SAME: (i64 noundef [[A:%.*]], i32 noundef [[B:%.*]], i32 noundef [[C:%.*]]) #[[ATTR0]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[A_ADDR:%.*]] = alloca i64, align 8
+// CHECK-NEXT:    [[B_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    [[C_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store i64 [[A]], i64* [[A_ADDR]], align 8
+// CHECK-NEXT:    store i32 [[B]], i32* [[B_ADDR]], align 4
+// CHECK-NEXT:    store i32 [[C]], i32* [[C_ADDR]], align 4
+// CHECK-NEXT:    [[TMP0:%.*]] = load i64, i64* [[A_ADDR]], align 8
+// CHECK-NEXT:    [[TMP1:%.*]] = load i32, i32* [[B_ADDR]], align 4
+// CHECK-NEXT:    [[CONV:%.*]] = sext i32 [[TMP1]] to i64
+// CHECK-NEXT:    [[ADD:%.*]] = add nsw i64 [[TMP0]], [[CONV]]
+// CHECK-NEXT:    [[TMP2:%.*]] = load i32, i32* [[C_ADDR]], align 4
+// CHECK-NEXT:    [[CONV1:%.*]] = sext i32 [[TMP2]] to i64
+// CHECK-NEXT:    [[ADD2:%.*]] = add nsw i64 [[ADD]], [[CONV1]]
+// CHECK-NEXT:    ret i64 [[ADD2]]
+//
+__attribute__((overloadable)) long test(long a, int b, int c) {
+  return a + b + c;
+}

clang/test/utils/update_cc_test_checks/mangled_names.test

 ## Running it again should implicitly add the function-signature flag due to UTC_ARGS:
 # RUN: %update_cc_test_checks %t.c
 # RUN: diff -u %t.c %S/Inputs/mangled_names.c.funcsig.expected
 ## Verify that running without the --function-signature flag removes the -SAME: lines:
 ## We have to remove the UTC_ARGS comment first:
 # RUN: grep -v UTC_ARGS %t.c > %t-no-args.c
 # RUN: %update_cc_test_checks %t-no-args.c
 # RUN: diff -u %t-no-args.c %S/Inputs/mangled_names.c.expected
+## Also try the --full-function-signature flag
+# RUN: %update_cc_test_checks %t.c --full-function-signature
+# RUN: diff -u %t.c %S/Inputs/mangled_names.c.fullfuncsig.expected

llvm/utils/UpdateTestChecks/asm.py

def get_run_handler(triple):
 
   return handler
 
 ##### Generator of assembly CHECK lines
 
 def add_checks(output_lines, comment_marker, prefix_list, func_dict,
                func_name, global_vars_seen_dict, is_filtered):
   # Label format is based on ASM string.
-  check_label_format = '{} %s-LABEL: %s%s%s'.format(comment_marker)
+  check_label_format = '{} %s-LABEL: %s%s%s%s'.format(comment_marker)
   return common.add_checks(output_lines, comment_marker, prefix_list, func_dict,
-                           func_name, check_label_format, True, False,
+                           func_name, check_label_format, True, False, False,
                            global_vars_seen_dict, is_filtered=is_filtered)

llvm/utils/UpdateTestChecks/common.py

 CHECK_RE = re.compile(r'^\s*(?://|[;#])\s*([^:]+?)(?:-NEXT|-NOT|-DAG|-LABEL|-SAME|-EMPTY)?:')
 
 UTC_ARGS_KEY = 'UTC_ARGS:'
 UTC_ARGS_CMD = re.compile(r'.*' + UTC_ARGS_KEY + '\s*(?P<cmd>.*)\s*$')
 UTC_ADVERT = 'NOTE: Assertions have been autogenerated by '
 UNUSED_NOTE = 'NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:'
 
 OPT_FUNCTION_RE = re.compile(
-    r'^(\s*;\s*Function\sAttrs:\s(?P<attrs>[\w\s():,]+?))?\s*define\s+(?:internal\s+)?[^@]*@(?P<func>[\w.$-]+?)\s*'
+    r'^(\s*;\s*Function\sAttrs:\s(?P<attrs>[\w\s():,]+?))?\s*define\s+(?P<funcdef_attrs_and_ret>[^@]*)@(?P<func>[\w.$-]+?)\s*'
     r'(?P<args_and_sig>\((\)|(.*?[\w.-]+?)\))[^{]*\{)\n(?P<body>.*?)^\}$',
     flags=(re.M | re.S))
 
 ANALYZE_FUNCTION_RE = re.compile(
     r'^\s*\'(?P<analysis>[\w\s-]+?)\'\s+for\s+function\s+\'(?P<func>[\w.$-]+?)\':'
     r'\s*\n(?P<body>.*)$',
     flags=(re.X | re.S))
 
def do_scrub(body, scrubber, scrubber_args, extra):
   if scrubber_args:
     local_args = copy.deepcopy(scrubber_args)
     local_args[0].extra_scrub = extra
     return scrubber(body, *local_args)
   return scrubber(body, *scrubber_args)
 
 # Build up a dictionary of all the function bodies.
 class function_body(object):
-  def __init__(self, string, extra, args_and_sig, attrs, func_name_separator):
+  def __init__(self, string, extra, funcdef_attrs_and_ret, args_and_sig, attrs, func_name_separator):
     self.scrub = string
     self.extrascrub = extra
+    self.funcdef_attrs_and_ret = funcdef_attrs_and_ret
     self.args_and_sig = args_and_sig
     self.attrs = attrs
     self.func_name_separator = func_name_separator
-  def is_same_except_arg_names(self, extrascrub, args_and_sig, attrs, is_backend):
+  def is_same_except_arg_names(self, extrascrub, funcdef_attrs_and_ret, args_and_sig, attrs, is_backend):
     arg_names = set()
     def drop_arg_names(match):
       arg_names.add(match.group(variable_group_in_ir_value_match))
       if match.group(attribute_group_in_ir_value_match):
         attr = match.group(attribute_group_in_ir_value_match)
       else:
         attr = ''
       return match.group(1) + attr + match.group(match.lastindex)
     def repl_arg_names(match):
       if match.group(variable_group_in_ir_value_match) is not None and match.group(variable_group_in_ir_value_match) in arg_names:
         return match.group(1) + match.group(match.lastindex)
       return match.group(1) + match.group(2) + match.group(match.lastindex)
+    if self.funcdef_attrs_and_ret != funcdef_attrs_and_ret:
+      return False
     if self.attrs != attrs:
       return False
     ans0 = IR_VALUE_RE.sub(drop_arg_names, self.args_and_sig)
     ans1 = IR_VALUE_RE.sub(drop_arg_names, args_and_sig)
     if ans0 != ans1:
       return False
     if is_backend:
       # Check without replacements, the replacements are not applied to the
for m in function_re.finditer(raw_tool_output):
       # colon: `foo:`. But, for example, in nvptx it is a brace: `foo(`. If is_backend is
       # False, just assume that separator is an empty string.
       if is_backend:
         # Use ':' as default separator.
         func_name_separator = m.group('func_name_separator') if 'func_name_separator' in m.groupdict() else ':'
       else:
         func_name_separator = ''
       attrs = m.group('attrs') if self._check_attributes else ''
+      funcdef_attrs_and_ret = m.group('funcdef_attrs_and_ret') if self._record_args else ''
       # Determine if we print arguments, the opening brace, or nothing after the
       # function name
       if self._record_args and 'args_and_sig' in m.groupdict():
         args_and_sig = scrub_body(m.group('args_and_sig').strip())
       elif 'args_and_sig' in m.groupdict():
         args_and_sig = '('
       else:
         args_and_sig = ''
for m in function_re.finditer(raw_tool_output):
             # 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 not None and
               (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)):
+               self._func_dict[prefix][func].attrs != attrs or
+               self._func_dict[prefix][func].funcdef_attrs_and_ret != funcdef_attrs_and_ret)):
             if self._func_dict[prefix][func].is_same_except_arg_names(
                 scrubbed_extra,
+                funcdef_attrs_and_ret,
                 args_and_sig,
                 attrs,
                 is_backend):
               self._func_dict[prefix][func].scrub = scrubbed_extra
               self._func_dict[prefix][func].args_and_sig = args_and_sig
             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 across RUN lines. We use None to
               # indicate that.
               self._func_dict[prefix][func] = None
         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, funcdef_attrs_and_ret,
+                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
 
return generalize_check_lines_common(lines, is_analyze, vars_seen,
                                        global_vars_seen, ir_nameless_values,
                                        IR_VALUE_RE, False)
 
 def generalize_asm_check_lines(lines, vars_seen, global_vars_seen):
   return generalize_check_lines_common(lines, False, vars_seen,
                                        global_vars_seen, asm_nameless_values,
                                        ASM_VALUE_RE, True)
 
-def add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name, check_label_format, is_backend, is_analyze, global_vars_seen_dict, is_filtered):
+def add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name, check_label_format, is_backend, is_analyze, check_funcdef_attrs_and_ret, global_vars_seen_dict, is_filtered):
   # prefix_exclusions are prefixes we cannot use to print the function because it doesn't exist in run lines that use these prefixes as well.
   prefix_exclusions = set()
   printed_prefixes = []
   for p in prefix_list:
     checkprefixes = p[0]
     # If not all checkprefixes of this run line produced the function we cannot check for it as it does not
     # exist for this run line. A subset of the check prefixes might know about the function but only because
     # other run lines created it.
for checkprefix in checkprefixes:
         global_vars_seen_dict[checkprefix] = {}
 
       global_vars_seen_before = [key for key in global_vars_seen.keys()]
 
       vars_seen = set()
       printed_prefixes.append(checkprefix)
       attrs = str(func_dict[checkprefix][func_name].attrs)
       attrs = '' if attrs == 'None' else attrs
+      if check_funcdef_attrs_and_ret:
+        funcdef_attrs_and_ret = '' + func_dict[checkprefix][func_name].funcdef_attrs_and_ret
+      else:
+        funcdef_attrs_and_ret = ''
+
       if attrs:
         output_lines.append('%s %s: Function Attrs: %s' % (comment_marker, checkprefix, attrs))
       args_and_sig = str(func_dict[checkprefix][func_name].args_and_sig)
       if args_and_sig:
         args_and_sig = generalize_check_lines([args_and_sig], is_analyze, vars_seen, global_vars_seen)[0]
       func_name_separator = func_dict[checkprefix][func_name].func_name_separator
       if '[[' in args_and_sig:
-        output_lines.append(check_label_format % (checkprefix, func_name, '', func_name_separator))
+        output_lines.append(check_label_format % (checkprefix, funcdef_attrs_and_ret, func_name, '', func_name_separator))
         output_lines.append('%s %s-SAME: %s' % (comment_marker, checkprefix, args_and_sig))
       else:
-        output_lines.append(check_label_format % (checkprefix, func_name, args_and_sig, func_name_separator))
+        output_lines.append(check_label_format % (checkprefix, funcdef_attrs_and_ret, func_name, args_and_sig, func_name_separator))
       func_body = str(func_dict[checkprefix][func_name]).splitlines()
       if not func_body:
         # We have filtered everything.
         continue
 
       # For ASM output, just emit the check lines.
       if is_backend:
         body_start = 1
for checkprefix in checkprefixes:
       # Remember new global variables we have not seen before
       for key in global_vars_seen:
         if key not in global_vars_seen_before:
           global_vars_seen_dict[checkprefix][key] = global_vars_seen[key]
       break
   return printed_prefixes
 
 def add_ir_checks(output_lines, comment_marker, prefix_list, func_dict,
-                  func_name, preserve_names, function_sig,
+                  func_name, preserve_names, function_sig, full_function_sig,
                   global_vars_seen_dict, is_filtered):
+  function_sig = function_sig or full_function_sig
   # Label format is based on IR string.
-  function_def_regex = 'define {{[^@]+}}' if function_sig else ''
+  if full_function_sig:
+    function_def_regex = 'define %s'
+  elif function_sig:
+    function_def_regex = 'define %s{{[^@]+}}'
+  else:
+    function_def_regex = '%s'
+
   check_label_format = '{} %s-LABEL: {}@%s%s%s'.format(comment_marker, function_def_regex)
   return add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name,
-                    check_label_format, False, preserve_names, global_vars_seen_dict,
+                    check_label_format, False, preserve_names,
+                    full_function_sig, global_vars_seen_dict,
                     is_filtered)
 
 def add_analyze_checks(output_lines, comment_marker, prefix_list, func_dict, func_name, is_filtered):
-  check_label_format = '{} %s-LABEL: \'%s%s%s\''.format(comment_marker)
+  check_label_format = '{} %s-LABEL: \'%s%s%s%s\''.format(comment_marker)
   global_vars_seen_dict = {}
   return add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name,
-                    check_label_format, False, True, global_vars_seen_dict,
+                    check_label_format, False, True, False, global_vars_seen_dict,
                     is_filtered)
 
 def build_global_values_dictionary(glob_val_dict, raw_tool_output, prefixes):
   for nameless_value in itertools.chain(ir_nameless_values, asm_nameless_values):
     if nameless_value.global_ir_prefix is None:
       continue
 
     lhs_re_str = nameless_value.global_ir_prefix + nameless_value.global_ir_prefix_regexp

llvm/utils/UpdateTestChecks/isel.py

def get_run_handler(triple):
 
   return handler
 
 ##### Generator of iSel CHECK lines
 
 def add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name,
                global_vars_seen_dict, is_filtered):
   # Label format is based on iSel string.
-  check_label_format = '{} %s-LABEL: %s%s%s'.format(comment_marker)
+  check_label_format = '{} %s-LABEL: %s%s%s%s'.format(comment_marker)
   return common.add_checks(output_lines, comment_marker, prefix_list, func_dict,
                            func_name, check_label_format, True, False,
-                           global_vars_seen_dict, is_filtered=is_filtered)
+                           False, global_vars_seen_dict, is_filtered=is_filtered)

llvm/utils/update_cc_test_checks.py

if not args.llvm_bin:
       args.clang = 'clang'
     else:
       args.clang = os.path.join(args.llvm_bin, 'clang')
   if not args.opt:
     if not args.llvm_bin:
       args.opt = 'opt'
     else:
       args.opt = os.path.join(args.llvm_bin, 'opt')
+  if args.full_function_signature and not args.function_signature:
+    args.function_signature = True
 
 
 def find_executable(executable):
   _, ext = os.path.splitext(executable)
   if sys.platform == 'win32' and ext != '.exe':
     executable = executable + '.exe'
 
   return shutil.which(executable)
def config():
   parser.add_argument(
       '--functions', nargs='+', help='A list of function name regexes. '
       'If specified, update CHECK lines for functions matching at least one regex')
   parser.add_argument(
       '--x86_extra_scrub', action='store_true',
       help='Use more regex for x86 matching to reduce diffs between various subtargets')
   parser.add_argument('--function-signature', action='store_true',
                       help='Keep function signature information around for the check line')
+  parser.add_argument('--full-function-signature', action='store_true',
+                      help='Keep function signature information (including return type) around for the check line')
   parser.add_argument('--check-attributes', action='store_true',
                       help='Check "Function Attributes" for functions')
   parser.add_argument('--check-globals', action='store_true',
                       help='Check global entries (global variables, metadata, attribute sets, ...) for functions')
   parser.add_argument('tests', nargs='+')
   args = common.parse_commandline_args(parser)
   infer_dependent_args(args)
 
if include_generated_funcs:
 
       # Now generate all the checks.
       def check_generator(my_output_lines, prefixes, func):
         if '-emit-llvm' in clang_args:
           return common.add_ir_checks(my_output_lines, '//',
                                       prefixes,
                                       func_dict, func, False,
                                       ti.args.function_signature,
+                                      ti.args.full_function_signature,
                                       global_vars_seen_dict,
                                       is_filtered=builder.is_filtered())
         else:
           return asm.add_checks(my_output_lines, '//',
                                 prefixes,
                                 func_dict, func, global_vars_seen_dict,
                                 is_filtered=builder.is_filtered())
 
else:
                   common.add_ir_checks(
                       output_lines,
                       '//',
                       filecheck_run_list,
                       func_dict,
                       mangled,
                       False,
                       args.function_signature,
+                      args.full_function_signature,
                       global_vars_seen_dict,
                       is_filtered=builder.is_filtered()))
               if line.rstrip('\n') == '//':
                 include_line = False
 
         if include_line:
           output_lines.append(line.rstrip('\n'))
 

llvm/utils/update_test_checks.py

if include_generated_funcs:
               lambda my_output_lines, prefixes, func: common.add_ir_checks(
                   my_output_lines,
                   ';',
                   prefixes,
                   func_dict,
                   func,
                   False,
                   args.function_signature,
+                  False,
                   global_vars_seen_dict,
                   is_filtered=builder.is_filtered())))
     else:
       # "Normal" mode.
       for input_line_info in ti.iterlines(output_lines):
         input_line = input_line_info.line
         args = input_line_info.args
         if is_in_function_start:
else:
               common.add_ir_checks(
                   output_lines,
                   ';',
                   prefix_list,
                   func_dict,
                   func_name,
                   args.preserve_names,
                   args.function_signature,
+                  False,
                   global_vars_seen_dict,
                   is_filtered=builder.is_filtered()))
           is_in_function_start = False
 
         m = common.IR_FUNCTION_RE.match(input_line)
         if m and not has_checked_pre_function_globals:
           if args.check_globals:
             generated_prefixes.extend(