[FileCheck] Fix isalpha/isalnum calls

[jdenny]

Summary

D79276 caused the following builder to fail:

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/23489

Specifically, FileCheck dumped stack in the following tests:

LLVM :: MC/Mips/micromips-jump-pc-region.s
LLVM :: MC/Mips/mips-jump-pc-region.s

Those tests contained characters encoded as 160 but that render (at
least for me in vim) like a single space (32). Those characters
appeared between the # and RUN: on several lines, and D79276
caused FileCheck to process those lines differently: RUN: is a
comment directive. As a result, D79276 caused FileCheck to start
calling is isalnum on those characters.

The problem is that FileCheck calls isalnum on type char without
casting to unsigned char first, so it sign-extends 160 beyond what
unsigned char or EOF can represent. C says that has undefined
behavior. This problem is general to FileCheck's prefix parsing and
so exists independently of D79276.

524457edbc3d fixed the above tests. This patch changes FileCheck to
use LLVM's replacements for ctype.h functions, and it adds tests for
cases that are representative with or without D79276.

[jhenderson]

LGTM, but with one suggestion.

llvm/test/FileCheck/bad-char.txt

33–36

Nit: I have a marginal preference for where CHECKs like this aren't shared to interleave them with the RUN lines, i.e.

RUN: ... --check-prefix=CHECK1

CHECK1: ...

RUN: ... --check-prefix=CHECK2

CHECK2: ...

It makes it slightly easier to follow each individual case. I might be inclined to move the input similarly, but I don't feel strongly about that one either way.

[thopre]

LGTM with James comment addressed. It seems I'm responsible for this mess since I introduced the first such patterns. My apologies for not paying enough attention to the type in the API.

[jdenny]

In D79810#2033794, @thopre wrote:

It seems I'm responsible for this mess since I introduced the first such patterns. My apologies for not paying enough attention to the type in the API.

Well, it seems no one who reviewed spotted it, and I doubt I would have thought of it either. :-)

llvm/test/FileCheck/bad-char.txt

33–36

I usually do that and originally wrote this one that way too. However, due to the symmetry among the cases here, I decided the way I have it now would be easier to read.

But it seems I'm outvoted two to one, and it don't feel strongly about it, especially given that my opinion might flip if the test evolves later. If no one objects, I'll change to the following form, with horizontal rules to guide the eye:

//------------------------------------------------
RUN: ... --check-prefix=CHECK1

input1 lines

CHECK1: ...

//------------------------------------------------
RUN: ... --check-prefix=CHECK2

input2 lines

CHECK2: ...

[efriedma]

Please use llvm::isAlpha/llvm::isAlnum instead of the functions from ctype.h; ctype.h is locale-dependent.

[jdenny]

Rebased.
Reformatted test as discussed.
Switched to llvm::isAlpha and llvm::isAlnum.

[jdenny]

In D79810#2034482, @efriedma wrote:

Please use llvm::isAlpha/llvm::isAlnum instead of the functions from ctype.h; ctype.h is locale-dependent.

Thanks! I wasn't aware of those.

[efriedma]

LGTM

llvm/lib/Support/FileCheck.cpp

 Expected<std::string> StringSubstitution::getResult() const {
   // Look up the value and escape it so that we can put it into the regex.
   Expected<StringRef> VarVal = Context->getPatternVarValue(FromStr);
   if (!VarVal)
     return VarVal.takeError();
   return Regex::escape(*VarVal);
 }
 
-bool Pattern::isValidVarNameStart(char C) { return C == '_' || isalpha(C); }
+bool Pattern::isValidVarNameStart(char C) {
+  return C == '_' || isalpha((unsigned char)C);
+}
 
 Expected<Pattern::VariableProperties>
 Pattern::parseVariable(StringRef &Str, const SourceMgr &SM) {
   if (Str.empty())
     return ErrorDiagnostic::get(SM, Str, "empty variable name");
 
   bool ParsedOneChar = false;
   unsigned I = 0;
   bool IsPseudo = Str[0] == '@';
 
   // Global vars start with '$'.
   if (Str[0] == '$' || IsPseudo)
     ++I;
 
   for (unsigned E = Str.size(); I != E; ++I) {
     if (!ParsedOneChar && !isValidVarNameStart(Str[I]))
       return ErrorDiagnostic::get(SM, Str, "invalid variable name");
 
     // Variable names are composed of alphanumeric characters and underscores.
-    if (Str[I] != '_' && !isalnum(Str[I]))
+    if (Str[I] != '_' && !isalnum((unsigned char)Str[I]))
       break;
     ParsedOneChar = true;
   }
 
   StringRef Name = Str.take_front(I);
   Str = Str.substr(I);
   return VariableProperties {Name, IsPseudo};
 }
FileCheckDiag::FileCheckDiag(const SourceMgr &SM,
   auto End = SM.getLineAndColumn(InputRange.End);
   InputStartLine = Start.first;
   InputStartCol = Start.second;
   InputEndLine = End.first;
   InputEndCol = End.second;
 }
 
 static bool IsPartOfWord(char c) {
-  return (isalnum(c) || c == '-' || c == '_');
+  return (isalnum((unsigned char)c) || c == '-' || c == '_');
 }
 
 Check::FileCheckType &Check::FileCheckType::setCount(int C) {
   assert(Count > 0 && "zero and negative counts are not supported");
   assert((C == 1 || Kind == CheckPlain) &&
          "count supported only for plain CHECK directives");
   Count = C;
   return *this;

llvm/test/FileCheck/bad-char.txt

+# This file contains characters that render as spaces (at least for me in vim)
+# but are encoded as 160.  Each is indicated with a "^" on the following line.
+# FileCheck used to call functions like isalnum on each without casting to
+# unsigned char first, so it sign-extended beyond what unsigned char or EOF can
+# represent.  C says that has undefined behavior, and it has caused stack dumps
+# under Windows.
+
+!BEFORE-PREFIX:
+^
+AFTER-PREFIX!:
+            ^
+BEFORE-VAR: [[!VAR:]]
+              ^
+AFTER-VAR: [[VAR!:]]
+                ^
+
+RUN: %ProtectFileCheckOutput \
+RUN: not FileCheck -check-prefix=BEFORE-PREFIX %s < /dev/null 2>&1 | \
+RUN:   FileCheck -check-prefix=ERR-EMPTY-CHECK %s
+
+RUN: %ProtectFileCheckOutput \
+RUN: not FileCheck -check-prefix=AFTER-PREFIX %s < /dev/null 2>&1 | \
+RUN:   FileCheck -check-prefix=ERR-NO-CHECK %s
+
+RUN: %ProtectFileCheckOutput \
+RUN: not FileCheck -check-prefix=BEFORE-VAR %s < /dev/null 2>&1 | \
+RUN:   FileCheck -check-prefix=ERR-BAD-VAR %s
+
+RUN: %ProtectFileCheckOutput \
+RUN: not FileCheck -check-prefix=AFTER-VAR %s < /dev/null 2>&1 | \
+RUN:   FileCheck -check-prefix=ERR-BAD-STRING-VAR %s
+
+ERR-EMPTY-CHECK: error: found empty check string
+ERR-NO-CHECK: error: no check strings found
+ERR-BAD-VAR: error: invalid variable name
+ERR-BAD-STRING-VAR: error: invalid name in string variable definition