[UpdateCCTestChecks] Support --check-globals

[jdenny]

Summary

This option is already supported by update_test_checks.py, but it can
also be useful in update_cc_test_checks.py. For example, I'd like to
use it in OpenMP offload codegen tests to check global variables like
.offload_maptypes*.

[jdoerfert]

LG, cool :)

[jdenny]

Fixed the reported test failure. (Sorry, I had accidentally put that fix in a later patch even though the associated test is in this patch.)

[jdenny]

In D104714#2833238, @jdoerfert wrote:

LG, cool :)

Thanks for the reviews.

[arichardson]

Thanks for working on this! We have some tests downstream that check globals and currently have to use // UTC_ARGS: --disable to manually retain them.

The other update script tests compare to an expected output file instead of using Filecheck directives. For larger tests the separate files are a lot more readable, whereas it doesn't really matter for this test.

clang/test/utils/update_cc_test_checks/check-globals.test

35

Running lit to verify that the output is valid could be something we might want to do for the other update script tests. But I guess using the scripts to generate tests that are checked it is usually sufficient.

llvm/utils/update_cc_test_checks.py

357

I feel like this line could do with a comment since it's not immediately obvious why it's needed as part of this commit.

[jdenny]

In D104714#2834696, @arichardson wrote:

Thanks for working on this!

Thanks for the review.

We have some tests downstream that check globals and currently have to use // UTC_ARGS: --disable to manually retain them.

Does that mean you manually maintain FileCheck directives there?

The other update script tests compare to an expected output file instead of using Filecheck directives. For larger tests the separate files are a lot more readable, whereas it doesn't really matter for this test.

I believe the only reason I used FileCheck directives in this case is because I didn't want to hard-code expected metadata and attributes, so I wanted regexes. However, I don't have a lot of experience writing checks for such constructs, so maybe hard-coding those is not brittle at all. If so, I'd be happy to switch to expected output files, which I agree are more readable.

clang/test/utils/update_cc_test_checks/check-globals.test

35

I don't know a general rule for when it's worthwhile and when it's not.

In this patch, I felt it was worthwhile because it was easy to think the directives looked fine without realizing declarations were in the wrong order (that happened to me at one point during development). I'm not sure someone modifying update_cc_test_checks.py and running this test suite is necessarily going to know which tests in other test suites should be re-generated using update_cc_test_checks.py and re-run to be sure all is well.

On the other hand, I think I could be convinced it's not worthwhile in my later patches in this series.

llvm/utils/update_cc_test_checks.py

357

Sure, I'll work on that.

[ggeorgakoudis]

LGTM too!

[jdenny]

Applied arichardson's suggestion: comment on exclusion of separator comments.

Rebased.

[jdenny]

In D104714#2835874, @ggeorgakoudis wrote:

LGTM too!

Thanks.

[thakis]

This seems to break tests: http://45.33.8.238/linux/49790/step_7.txt

[thakis]

Oh I see this assumes that lit.site.cfg.py is below clang_obj_root. I guess that's my problem, looking…

[jdenny]

In D104714#2841229, @thakis wrote:

Oh I see this assumes that lit.site.cfg.py is below clang_obj_root. I guess that's my problem, looking…

Let me know if the patch isn't general enough, or if I can help in some other way.

Thanks.

[thakis]

I landed a fix attempt for my thing in fda790fbfa0cba426d5e3787429a51e09ec64c6d .

[jdenny]

In D104714#2841304, @thakis wrote:

I landed a fix attempt for my thing in fda790fbfa0cba426d5e3787429a51e09ec64c6d .

Thanks!

[thakis]

Another thing: https://reviews.llvm.org/harbormaster/unit/view/789318/

# command stderr:
/var/lib/buildkite-agent/builds/llvm-project/clang/test/utils/update_cc_test_checks/check-globals.test:83:10: error: LIT-RUN: expected string not found in input
LIT-RUN: PASS: {{.*}} norm.c

Input was:
<<<<<<
            1: lit.py: /var/lib/buildkite-agent/builds/llvm-project/llvm/utils/lit/lit/llvm/config.py:436: note: using clang: /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang 
            2: -- Testing: 2 tests, 1 workers -- 
            3: PASS: update_cc_test_checks.py example :: norm.c (1 of 2) 
            4: PASS: update_cc_test_checks.py example :: igf.c (2 of 2)

Looks like the test expects order igf.c, norm.c while the output is the other way round. Should this just use LIT-RUN-DAG so that the order doesn't matter, or should we add a sorted() somewhere that the output is deterministic?

[jdenny]

In D104714#2841457, @thakis wrote:

Another thing: https://reviews.llvm.org/harbormaster/unit/view/789318/

# command stderr:
/var/lib/buildkite-agent/builds/llvm-project/clang/test/utils/update_cc_test_checks/check-globals.test:83:10: error: LIT-RUN: expected string not found in input
LIT-RUN: PASS: {{.*}} norm.c

Input was:
<<<<<<
            1: lit.py: /var/lib/buildkite-agent/builds/llvm-project/llvm/utils/lit/lit/llvm/config.py:436: note: using clang: /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang 
            2: -- Testing: 2 tests, 1 workers -- 
            3: PASS: update_cc_test_checks.py example :: norm.c (1 of 2) 
            4: PASS: update_cc_test_checks.py example :: igf.c (2 of 2)

Looks like the test expects order igf.c, norm.c while the output is the other way round. Should this just use LIT-RUN-DAG so that the order doesn't matter, or should we add a sorted() somewhere that the output is deterministic?

I pushed cc60fa2685bd to fix that.

[tstellar]

It looks like this patch breaks the utils/update_cc_test_checks/check-globals.test on stand-alone builds of clang, because it hard codes lit to lit.py in the llvm source tree. I think it should using the lit passed to -DLLVM_EXTERNAL_LIT= (if that option was specified).

[tstellar]

In D104714#2872826, @tstellar wrote:

It looks like this patch breaks the utils/update_cc_test_checks/check-globals.test on stand-alone builds of clang, because it hard codes lit to lit.py in the llvm source tree. I think it should using the lit passed to -DLLVM_EXTERNAL_LIT= (if that option was specified).

This patch fixes the failure for me: D105873.

clang/test/utils/update_cc_test_checks/Inputs/check-globals.c

+// First, make sure --check-globals doesn't crash on a non-FileChecked command.
+// RUN: true
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+void foo() {
+  static int i, j;
+}
+void bar() {
+  static int i, j;
+}

clang/test/utils/update_cc_test_checks/Inputs/lit.cfg.example

+import lit
+lit_config.load_config(
+    config, os.path.join(lit_config.params.get('clang_obj_root'),
+                         "test/lit.site.cfg.py"))
+config.name = 'update_cc_test_checks.py example'
+config.suffixes = ['.c', '.cpp']
+config.test_format = lit.formats.ShTest(execute_external=False)
+config.test_source_root = os.path.dirname(__file__)
+config.test_exec_root = os.path.dirname(__file__)

clang/test/utils/update_cc_test_checks/check-globals.test

+RUN: rm -rf %t && mkdir %t
+
+# Check --check-globals in normal mode and in --include-generated-funcs mode.
+
+RUN: cp %S/Inputs/check-globals.c %t/norm.c
+RUN: %update_cc_test_checks %t/norm.c --check-globals
+RUN: FileCheck %s --input-file=%t/norm.c --match-full-lines -strict-whitespace \
+RUN:   -check-prefixes=BOTH,NRM
+
+RUN: cp %S/Inputs/check-globals.c %t/igf.c
+RUN: %update_cc_test_checks %t/igf.c --check-globals --include-generated-funcs
+RUN: FileCheck %s --input-file=%t/igf.c --match-full-lines -strict-whitespace \
+RUN:   -check-prefixes=BOTH,IGF
+
+# Check that repeating doesn't change it, such as duplicating '//.' occurrences.
+
+RUN: cp %t/norm.c %t/norm-again.c
+RUN: %update_cc_test_checks %t/norm-again.c --check-globals
+RUN: diff -u %t/norm.c %t/norm-again.c
+RUN: rm %t/norm-again.c
+
+RUN: cp %t/igf.c %t/igf-again.c
+RUN: %update_cc_test_checks %t/igf-again.c --check-globals \
+RUN:     --include-generated-funcs
+RUN: diff -u %t/igf.c %t/igf-again.c
+RUN: rm %t/igf-again.c
+
+# Check that the generated directives actually work correctly.  For example,
+# they're not in the wrong order.
+
+RUN: cp %S/Inputs/lit.cfg.example %t/lit.cfg
+# Show lit failures while avoiding confusing FileCheck input dump nesting.
+RUN: %lit %t
+# Lit was successful.  Sanity-check the results.
+RUN: %lit %t 2>&1 | FileCheck -check-prefix=LIT-RUN %s
+
+END.
+
+  BOTH-NOT:{{.}}
+       NRM:// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-globals
+       IGF:// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-globals --include-generated-funcs
+ BOTH-NEXT:// {{.*}}
+ BOTH-NEXT:// RUN: true
+ BOTH-NEXT:// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+BOTH-EMPTY:
+  IGF-NEXT:void foo() {
+  IGF-NEXT:  static int i, j;
+  IGF-NEXT:}
+  IGF-NEXT:void bar() {
+  IGF-NEXT:  static int i, j;
+  IGF-NEXT:}
+ BOTH-NEXT://.
+ BOTH-NEXT:// CHECK: @foo.i = internal global i32 0, align 4
+ BOTH-NEXT:// CHECK: @foo.j = internal global i32 0, align 4
+ BOTH-NEXT:// CHECK: @bar.i = internal global i32 0, align 4
+ BOTH-NEXT:// CHECK: @bar.j = internal global i32 0, align 4
+ BOTH-NEXT://.
+ BOTH-NEXT:// CHECK-LABEL: @foo(
+ BOTH-NEXT:// CHECK-NEXT:  entry:
+ BOTH-NEXT:// CHECK-NEXT:    ret void
+ BOTH-NEXT://
+  NRM-NEXT:void foo() {
+  NRM-NEXT:  static int i, j;
+  NRM-NEXT:}
+  IGF-NEXT://
+ BOTH-NEXT:// CHECK-LABEL: @bar(
+ BOTH-NEXT:// CHECK-NEXT:  entry:
+ BOTH-NEXT:// CHECK-NEXT:    ret void
+ BOTH-NEXT://
+  NRM-NEXT:void bar() {
+  NRM-NEXT:  static int i, j;
+  NRM-NEXT:}
+ BOTH-NEXT://.
+ BOTH-NEXT:// CHECK: attributes {{.*}}
+ BOTH-NEXT://.
+ BOTH-NEXT:// CHECK: !0 = {{.*}}
+ BOTH-NEXT:// CHECK: !1 = {{.*}}
+ BOTH-NEXT://.
+  BOTH-NOT:{{.}}
+
+LIT-RUN: Testing: 2 tests
+LIT-RUN: PASS: {{.*}} igf.c
+LIT-RUN: PASS: {{.*}} norm.c

clang/test/utils/update_cc_test_checks/lit.local.cfg

 
 clang_path = os.path.join(config.clang_tools_dir, 'clang')
 extra_args = '--clang ' + shell_quote(clang_path)
 opt_path = os.path.join(config.llvm_tools_dir, 'opt')
 extra_args += ' --opt ' + shell_quote(opt_path)
 script_path = os.path.join(config.llvm_src_root, 'utils',
                            'update_cc_test_checks.py')
 assert os.path.isfile(script_path)
+lit = shell_quote(os.path.join(config.llvm_src_root, 'utils', 'lit', 'lit.py'))
+python = shell_quote(config.python_executable)
 config.substitutions.append(
     ('%update_cc_test_checks', "%s %s %s" % (
-        shell_quote(config.python_executable), shell_quote(script_path),
-        extra_args)))
+        python, shell_quote(script_path), extra_args)))
 config.substitutions.append(
     ('%clang_tools_dir', shell_quote(config.clang_tools_dir)))
+config.substitutions.append(
+    ('%lit', "%s %s -Dclang_obj_root=%s -j1 -vv" % (
+        python, lit, shell_quote(config.clang_obj_root))))

llvm/utils/UpdateTestChecks/common.py

def add_global_checks(glob_val_dict, comment_marker, prefix_list, output_lines, global_vars_seen_dict, is_analyze, is_before_functions):
   for nameless_value in nameless_values:
     if nameless_value.global_ir_prefix is None:
         continue
     if nameless_value.is_before_functions != is_before_functions:
         continue
     for p in prefix_list:
       global_vars_seen = {}
       checkprefixes = p[0]
+      if checkprefixes is None:
+        continue
       for checkprefix in checkprefixes:
         if checkprefix in global_vars_seen_dict:
             global_vars_seen.update(global_vars_seen_dict[checkprefix])
         else:
             global_vars_seen_dict[checkprefix] = {}
         if (checkprefix, nameless_value.check_prefix) in printed_prefixes:
           break
         if not glob_val_dict[checkprefix]:

llvm/utils/update_cc_test_checks.py

parser.add_argument(
       '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('--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 not distutils.spawn.find_executable(args.clang):
     print('Please specify --llvm-bin or --clang', file=sys.stderr)
     sys.exit(1)
 
for prefixes, args, extra_commands, triple_in_cmd in run_list:
       # mangled names. Forward all clang args for now.
       for k, v in get_line2spell_and_mangled(ti.args, clang_args).items():
         line2spell_and_mangled_list[k].append(v)
 
     func_dict = builder.finish_and_get_func_dict()
     global_vars_seen_dict = {}
     prefix_set = set([prefix for p in filecheck_run_list for prefix in p[0]])
     output_lines = []
+    has_checked_pre_function_globals = False
 
     include_generated_funcs = common.find_arg_in_test(ti,
                                                       lambda args: ti.args.include_generated_funcs,
                                                       '--include-generated-funcs',
                                                       True)
 
     if include_generated_funcs:
       # Generate the appropriate checks for each function.  We need to emit
if include_generated_funcs:
                                func_dict, func, False,
                                ti.args.function_signature,
                                global_vars_seen_dict)
         else:
           asm.add_asm_checks(my_output_lines, '//',
                              prefixes,
                              func_dict, func)
 
+      if ti.args.check_globals:
+        common.add_global_checks(builder.global_var_dict(), '//', run_list,
+                                 output_lines, global_vars_seen_dict, True,
+                                 True)
       common.add_checks_at_end(output_lines, filecheck_run_list, builder.func_order(),
                                '//', lambda my_output_lines, prefixes, func:
                                check_generator(my_output_lines,
                                                prefixes, func))
     else:
       # Normal mode.  Put checks before each source function.
       for line_info in ti.iterlines(output_lines):
         idx = line_info.line_number
         line = line_info.line
         args = line_info.args
         include_line = True
         m = common.CHECK_RE.match(line)
         if m and m.group(1) in prefix_set:
           continue  # Don't append the existing CHECK lines
+        # Skip special separator comments added by commmon.add_global_checks.
+        if line.strip() == '//' + common.SEPARATOR:
+          continue
         if idx in line2spell_and_mangled_list:
           added = set()
           for spell, mangled in line2spell_and_mangled_list[idx]:
             # One line may contain multiple function declarations.
             # Skip if the mangled name has been added before.
             # The line number may come from an included file,
             # we simply require the spelling name to appear on the line
             # to exclude functions from other files.
             if mangled in added or spell not in line:
               continue
             if args.functions is None or any(re.search(regex, spell) for regex in args.functions):
               last_line = output_lines[-1].strip()
               while last_line == '//':
                 # Remove the comment line since we will generate a new  comment
                 # line as part of common.add_ir_checks()
                 output_lines.pop()
                 last_line = output_lines[-1].strip()
+              if ti.args.check_globals and not has_checked_pre_function_globals:
+                common.add_global_checks(builder.global_var_dict(), '//',
+                                         run_list, output_lines,
+                                         global_vars_seen_dict, True, True)
+                has_checked_pre_function_globals = True
               if added:
                 output_lines.append('//')
               added.add(mangled)
               common.add_ir_checks(output_lines, '//', filecheck_run_list, func_dict, mangled,
                                    False, args.function_signature, global_vars_seen_dict)
               if line.rstrip('\n') == '//':
                 include_line = False
 
         if include_line:
           output_lines.append(line.rstrip('\n'))
 
+    if ti.args.check_globals:
+      common.add_global_checks(builder.global_var_dict(), '//', run_list,
+                               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])
 
   return 0
 
 
 if __name__ == '__main__':
   sys.exit(main())