[FileCheck] Fix parsing empty global and pseudo variable names

[kovdan01]

Reland #82595 with fixes of build failures related to colored output.
See https://lab.llvm.org/buildbot/#/builders/139/builds/60549
Force -color=0 to avoid colored output if autodetect does not work properly.
Original commit message below.

In Pattern::parseVariable, for global variables (those starting with '$')
and for pseudo variables (those starting with '@') the first character is
consumed before actual variable name parsing. If the name is empty, it leads
to out-of-bound access to the corresponding StringRef.

This patch adds an if statement against the case described.

[llvmbot]

@llvm/pr-subscribers-testing-tools

Author: Daniil Kovalev (kovdan01)

Changes

Reland #82595 with fixes of build failures related to colored output.
See https://lab.llvm.org/buildbot/#/builders/139/builds/60549
Force -color=0 to avoid colored output if autodetect does not work properly.
Original commit message below.

In Pattern::parseVariable, for global variables (those starting with '$')
and for pseudo variables (those starting with '@') the first character is
consumed before actual variable name parsing. If the name is empty, it leads
to out-of-bound access to the corresponding StringRef.

This patch adds an if statement against the case described.

---

Full diff: https://github.com/llvm/llvm-project/pull/83667.diff

2 Files Affected:

• (modified) llvm/lib/FileCheck/FileCheck.cpp (+6)
• (added) llvm/test/FileCheck/empty-variable-name.txt (+32)

diff --git a/llvm/lib/FileCheck/FileCheck.cpp b/llvm/lib/FileCheck/FileCheck.cpp
index 6d3a2b9cf46f7c..8f80a69c4abd3a 100644
--- a/llvm/lib/FileCheck/FileCheck.cpp
+++ b/llvm/lib/FileCheck/FileCheck.cpp
@@ -297,6 +297,12 @@ Pattern::parseVariable(StringRef &Str, const SourceMgr &SM) {
   if (Str[0] == '$' || IsPseudo)
     ++I;
 
+  if (I == Str.size())
+    return ErrorDiagnostic::get(SM, Str.slice(I, StringRef::npos),
+                                StringRef("empty ") +
+                                    (IsPseudo ? "pseudo " : "global ") +
+                                    "variable name");
+
   if (!isValidVarNameStart(Str[I++]))
     return ErrorDiagnostic::get(SM, Str, "invalid variable name");
 
diff --git a/llvm/test/FileCheck/empty-variable-name.txt b/llvm/test/FileCheck/empty-variable-name.txt
new file mode 100644
index 00000000000000..44cdd2c0e677ba
--- /dev/null
+++ b/llvm/test/FileCheck/empty-variable-name.txt
@@ -0,0 +1,32 @@
+a
+
+; RUN: not FileCheck -color=0 -input-file %s %s 2>&1 | \
+; RUN:   FileCheck -check-prefix CHECK-ERROR -DDIR=%S \
+; RUN:   --match-full-lines --strict-whitespace %s
+
+; CHECK:        a[[]]
+; CHECK-ERROR:[[DIR]]{{/|\\}}empty-variable-name.txt:7:13: error: empty variable name
+; CHECK-ERROR-NEXT:; CHECK: a{{\[\[\]\]}}
+;      CHECK-ERROR-NEXT:            ^
+
+b
+
+; RUN: not FileCheck -color=0 -input-file %s -check-prefix CHECK-PSEUDO %s 2>&1 | \
+; RUN:   FileCheck -check-prefix CHECK-ERROR-PSEUDO -DDIR=%S \
+; RUN:   --match-full-lines --strict-whitespace %s
+
+; CHECK-PSEUDO: b[[@]]
+; CHECK-ERROR-PSEUDO:[[DIR]]{{/|\\}}empty-variable-name.txt:18:21: error: empty pseudo variable name
+; CHECK-ERROR-PSEUDO-NEXT:; CHECK-PSEUDO: b{{\[\[@\]\]}}
+;      CHECK-ERROR-PSEUDO-NEXT:                    ^
+
+c
+
+; RUN: not FileCheck -color=0 -input-file %s -check-prefix CHECK-GLOBAL %s 2>&1 | \
+; RUN:   FileCheck -check-prefix CHECK-ERROR-GLOBAL -DDIR=%S \
+; RUN:   --match-full-lines --strict-whitespace %s
+
+; CHECK-GLOBAL: c[[$]]
+; CHECK-ERROR-GLOBAL:[[DIR]]{{/|\\}}empty-variable-name.txt:29:21: error: empty global variable name
+; CHECK-ERROR-GLOBAL-NEXT:; CHECK-GLOBAL: c{{\[\[\$\]\]}}
+;       CHECK-ERROR-GLOBAL-NEXT:                    ^

[MaskRay]

Does the failing build bot set FILECHECK_OPTS=--color? The idiom is to set %ProtectFileCheckOutput.

I believe -color=0 is not needed.

[kovdan01]

Yes, see FILECHECK_OPTS=-dump-input-filter=all -vv -color here: https://lab.llvm.org/buildbot/#/builders/139/builds/60549/steps/2/logs/stdio

It's also clear that colors are enabled from the failing output.

IMHO even if the bot shouldn't set this and if it should be changed, the -color=0 is still nice to have since if a test relies on assumption that colors are disabled - we'd better have the option explicitly disabled.

[MaskRay]

I disagree. There are many CHECK.*error: tests and they don't use -color=0 at all.

[kovdan01]

OK, it makes sense. Deleted -color=0 and used %ProtectFileCheckOutput instead. See 3bbb7c1