[UpdateTestChecks] Emit warning when invalid value for -check-prefix(es) option

[xbolva00]

Summary

The script is silent for the following issue:
FileCheck %s -check-prefix=CHECK,POPCOUNT

FileCheck will catch it later, but I think we can warn here too.

Now it warns:
./update_llc_test_checks.py file.ll

WARNING: Supplied prefix 'CHECK,POPCOUNT' is invalid. Prefix must contain only alphanumeric characters, hyphens and underscores. Did you mean --check-prefixes=CHECK,POPCOUNT?

[xbolva00]

lebedev.ri hit this recently so I added him as reviewer.

[lebedev.ri]

Thank you, i have stumbled into this a few times :)

To be noted, the problem is: --check-prefix=A,B,C.
But i'm not sure about --check-prefix=A --check-prefix=B --check-prefix=C.

[xbolva00]

More precise checker.

[xbolva00]

In D64589#1581406, @lebedev.ri wrote:

Thank you, i have stumbled into this a few times :)

To be noted, the problem is: --check-prefix=A,B,C.
But i'm not sure about --check-prefix=A --check-prefix=B --check-prefix=C.

Thanks, I fixed it.

[xbolva00]

-check-prefix=A,B -check-prefix=C

^ This won't be detected, not sure if worth to match. What do you think?

[lebedev.ri]

I'm not confident that will do the right thing. (i expect this to have some false-negatives, iff it is allowed to specify -check-prefix= more than once)
I see two paths:

1. split the runline by whitespace, and check every element that starts with -check-prefix= that it does not contain ,. Will not have false-positives and no false-negatives, but kinda ugly.
2. Revert to the previous diff, complain if there are any -check-prefix= IFF we had more than a single prefix; the warning will need to be changed - it will have false-positives - so it is more of a stylistic guideline.

[xbolva00]

Improved.

[xbolva00]

In D64589#1581499, @lebedev.ri wrote:

I'm not confident that will do the right thing. (i expect this to have some false-negatives, iff it is allowed to specify -check-prefix= more than once)
I see two paths:

1. split the runline by whitespace, and check every element that starts with -check-prefix= that it does not contain ,. Will not have false-positives and no false-negatives, but kinda ugly.
2. Revert to the previous diff, complain if there are any -check-prefix= IFF we had more than a single prefix; the warning will need to be changed - it will have false-positives - so it is more of a stylistic guideline.

I implemented 1. option. Thanks for ideas.

[xbolva00]

Renamed variable.

[lebedev.ri]

Nice, thanks for looking into this.
I think this is good, @RKSimon ?

[RKSimon]

is there anyway that we could move this to common.py so we can reuse it in the other update scripts?

[xbolva00]

Moved to common.

[xbolva00]

In D64589#1582936, @RKSimon wrote:

is there anyway that we could move this to common.py so we can reuse it in the other update scripts?

Thanks, I forgot about mir. Moved to common, now it warns for broken LLC/MIR prefixes.

[xbolva00]

Also check IR.

[RKSimon]

Please can you add this to update_analyze_test_checks.py and update_mca_test_checks.py as well? update_cc_test_checks.py maybe - not sure what state that is in though tbh.

[xbolva00]

Add checker to all update scripts.

[xbolva00]

Any further comments?

[gbedwell]

utils/UpdateTestChecks/common.py

272 ↗

(On Diff #209590)

I doubt whether it could ever cause an issue in practice, but it's probably safer to specify maxsplit here.

prefix = part.split('=', 1)[1]

If we somehow ever ended up with a string like '--check-prefix=foo=bar' then we'd end up with 'foo=bar' in element 1 rather than 'foo' in element 1 and 'bar' in element 2 which would subsequently be silently ignored. Alternatively you could just check that the number of elements returned from the split is exactly two and raise an error or warning if not.

[xbolva00]

Reworked to use regex to check prefixes.

[xbolva00]

@gbedwell Thanks for ideas!

I decided to take the regex [0] which FileCheck uses to check prefixes and apply it here.

[0] https://github.com/llvm/llvm-project/blob/2a7f5204602938ae89b0860e9412603d1951d945/llvm/lib/Support/FileCheck.cpp

[xbolva00]

Handle empty prefixes.

[xbolva00]

@lebedev.ri @RKSimon

FileCheck says
"Prefixes must be unique, *start with a letter* and contain only alphanumeric characters, hyphens and underscores"

but it does not enforce it in RE, should we enforce it here?

[xbolva00]

Check if prefix is unique.

[gbedwell]

I've tried the latest update and it seems to work well for me. The only suggestion I'd make at this stage is adding a message like (did you mean --check-prefixes?) to the end of the warning if a comma is spotted in the prefix string.

--check-prefix=FOO,BAR

gives a fairly generic warning message now:

WARNING: Supplied prefix 'FOO,BAR' is invalid. Prefix must start with a letter and contain only alphanumeric characters, hyphens and underscores.

which I can believe that if I was new to filecheck would probably cause me to spend more time debugging the issue than I'd like.

utils/UpdateTestChecks/common.py

287 ↗

(On Diff #209801)

A very nitpicky and minor style nit, but you can contain single quotes in double quoted strings and double quotes in single quotes strings, so it may be more readable like this as you don't have to escape anything.

print('WARNING: Supplied prefix "%s" is not unique in prefix list.'

or:

print("WARNING: Supplied prefix '%s' is not unique in prefix list."

[xbolva00]

Updated. Hint added.

[xbolva00]

utils/UpdateTestChecks/common.py

287 ↗

(On Diff #209801)

+1

Yeah, this looks better.

[xbolva00]

Revert unneeded change.

[xbolva00]

Ping

[xbolva00]

Ping

[RKSimon]

LGTM - cheers!

[gbedwell]

In D64589#1604206, @RKSimon wrote:

LGTM - cheers!

Sorry for the delay. I've been out of the office a little bit recently!
LGTM too.

[lebedev.ri]

Thank you for looking into this!

[xbolva00]

Thank you all for comments and ideas

llvm/trunk/utils/UpdateTestChecks/common.py

if sys.version_info[0] > 2:
       stdout = stdout.decode()
   # Fix line endings to unix CR style.
   return stdout.replace('\r\n', '\n')
 
 ##### LLVM IR parser
 
 RUN_LINE_RE = re.compile('^\s*[;#]\s*RUN:\s*(.*)$')
 CHECK_PREFIX_RE = re.compile('--?check-prefix(?:es)?[= ](\S+)')
+PREFIX_RE = re.compile('^[a-zA-Z0-9_-]+$')
 CHECK_RE = re.compile(r'^\s*[;#]\s*([^:]+?)(?:-NEXT|-NOT|-DAG|-LABEL)?:')
 
 OPT_FUNCTION_RE = re.compile(
     r'^\s*define\s+(?:internal\s+)?[^@]*@(?P<func>[\w-]+?)\s*\('
     r'(\s+)?[^)]*[^{]*\{\n(?P<body>.*?)^\}$',
     flags=(re.M | re.S))
 
 ANALYZE_FUNCTION_RE = re.compile(

 def add_ir_checks(output_lines, comment_marker, prefix_list, func_dict, func_name):
   # Label format is based on IR string.
   check_label_format = '{} %s-LABEL: @%s('.format(comment_marker)
   add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name, check_label_format, False, False)
 
 def add_analyze_checks(output_lines, comment_marker, prefix_list, func_dict, func_name):
   check_label_format = '{} %s-LABEL: \'%s\''.format(comment_marker)
   add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name, check_label_format, False, True)
+
+
+def check_prefix(prefix):
+  if not PREFIX_RE.match(prefix):
+        hint = ""
+        if ',' in prefix:
+          hint = " Did you mean '--check-prefixes=" + prefix + "'?"
+        print(("WARNING: Supplied prefix '%s' is invalid. Prefix must contain only alphanumeric characters, hyphens and underscores." + hint) %
+              (prefix), file=sys.stderr)
+
+
+def verify_filecheck_prefixes(fc_cmd):
+  fc_cmd_parts = fc_cmd.split()
+  for part in fc_cmd_parts:
+    if "check-prefix=" in part:
+      prefix = part.split('=', 1)[1]
+      check_prefix(prefix)
+    elif "check-prefixes=" in part:
+      prefixes = part.split('=', 1)[1].split(',')
+      for prefix in prefixes:
+        check_prefix(prefix)
+        if prefixes.count(prefix) > 1:
+          print("WARNING: Supplied prefix '%s' is not unique in the prefix list." %
+                (prefix,), file=sys.stderr)

llvm/trunk/utils/update_analyze_test_checks.py

for test in test_paths:
     if args.verbose:
       print('Found %d RUN lines:' % (len(run_lines),), file=sys.stderr)
       for l in run_lines:
         print('  RUN: ' + l, file=sys.stderr)
 
     prefix_list = []
     for l in run_lines:
       (tool_cmd, filecheck_cmd) = tuple([cmd.strip() for cmd in l.split('|', 1)])
+      common.verify_filecheck_prefixes(filecheck_cmd)
 
       if not tool_cmd.startswith(opt_basename + ' '):
         print('WARNING: Skipping non-%s RUN line: %s' % (opt_basename, l), file=sys.stderr)
         continue
 
       if not filecheck_cmd.startswith('FileCheck '):
         print('WARNING: Skipping non-FileChecked RUN line: ' + l, file=sys.stderr)
         continue

llvm/trunk/utils/update_cc_test_checks.py

for l in run_lines:
       if clang_args[0] not in SUBST:
         print('WARNING: Skipping non-clang RUN line: ' + l, file=sys.stderr)
         continue
       clang_args[0:1] = SUBST[clang_args[0]]
       clang_args = [filename if i == '%s' else i for i in clang_args] + args.clang_args
 
       # Extract -check-prefix in FileCheck args
       filecheck_cmd = commands[-1]
+      common.verify_filecheck_prefixes(filecheck_cmd)
       if not filecheck_cmd.startswith('FileCheck '):
         print('WARNING: Skipping non-FileChecked RUN line: ' + l, file=sys.stderr)
         continue
       check_prefixes = [item for m in common.CHECK_PREFIX_RE.finditer(filecheck_cmd)
                                for item in m.group(1).split(',')]
       if not check_prefixes:
         check_prefixes = ['CHECK']
       run_list.append((check_prefixes, clang_args, triple_in_cmd))

llvm/trunk/utils/update_llc_test_checks.py

for l in run_lines:
       march_in_cmd = None
       m = common.MARCH_ARG_RE.search(llc_cmd)
       if m:
         march_in_cmd = m.groups()[0]
 
       filecheck_cmd = ''
       if len(commands) > 1:
         filecheck_cmd = commands[1]
+      common.verify_filecheck_prefixes(filecheck_cmd)
       if not llc_cmd.startswith('llc '):
         print('WARNING: Skipping non-llc RUN line: ' + l, file=sys.stderr)
         continue
 
       if not filecheck_cmd.startswith('FileCheck '):
         print('WARNING: Skipping non-FileChecked RUN line: ' + l, file=sys.stderr)
         continue
 

llvm/trunk/utils/update_mca_test_checks.py

def _get_run_infos(run_lines, args):
   for run_line in run_lines:
     try:
       (tool_cmd, filecheck_cmd) = tuple([cmd.strip()
                                         for cmd in run_line.split('|', 1)])
     except ValueError:
       _warn('could not split tool and filecheck commands: {}'.format(run_line))
       continue
 
+    common.verify_filecheck_prefixes(filecheck_cmd)
     tool_basename = os.path.splitext(os.path.basename(args.llvm_mca_binary))[0]
 
     if not tool_cmd.startswith(tool_basename + ' '):
       _warn('skipping non-{} RUN line: {}'.format(tool_basename, run_line))
       continue
 
     if not filecheck_cmd.startswith('FileCheck '):
       _warn('skipping non-FileCheck RUN line: {}'.format(run_line))

llvm/trunk/utils/update_mir_test_checks.py

 
 def build_run_list(test, run_lines, verbose=False):
     run_list = []
     all_prefixes = []
     for l in run_lines:
         commands = [cmd.strip() for cmd in l.split('|', 1)]
         llc_cmd = commands[0]
         filecheck_cmd = commands[1] if len(commands) > 1 else ''
+        common.verify_filecheck_prefixes(filecheck_cmd)
 
         if not llc_cmd.startswith('llc '):
             warn('Skipping non-llc RUN line: {}'.format(l), test_file=test)
             continue
         if not filecheck_cmd.startswith('FileCheck '):
             warn('Skipping non-FileChecked RUN line: {}'.format(l),
                  test_file=test)
             continue

llvm/trunk/utils/update_test_checks.py

for test in test_paths:
     if args.verbose:
       print('Found %d RUN lines:' % (len(run_lines),), file=sys.stderr)
       for l in run_lines:
         print('  RUN: ' + l, file=sys.stderr)
 
     prefix_list = []
     for l in run_lines:
       (tool_cmd, filecheck_cmd) = tuple([cmd.strip() for cmd in l.split('|', 1)])
-
+      common.verify_filecheck_prefixes(filecheck_cmd)
       if not tool_cmd.startswith(opt_basename + ' '):
         print('WARNING: Skipping non-%s RUN line: %s' % (opt_basename, l), file=sys.stderr)
         continue
 
       if not filecheck_cmd.startswith('FileCheck '):
         print('WARNING: Skipping non-FileChecked RUN line: ' + l, file=sys.stderr)
         continue