[PATCH] D99330: [clang-tidy] [test] Tweak a regex pattern to accommodate for quirks in MSYS based grep

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added a reviewer: rnk.
Herald added a subscriber: xazax.hun.
mstorsjo requested review of this revision.
Herald added a project: clang-tools-extra.

Currently on Windows, 'not' is a regular win32 executable, which
parses command line parameters according to the canonical rules of
the platform. 'grep' can be a tool running either on the regular
win32 runtime, or on the MSYS runtime. If based on the MSYS runtime,
it parses the command line parameters quite significantly differently.

When 'not' invokes 'grep', it quotes the arguments differently than
how lit's TestRunner does, making this particular case of regex
pattern work with MSYS grep when invoked by 'not', but fails if
invoked by TestRunner directly.

(If TestRunner is modified to quote arguments like 'not' does, a
whole bunch of other tests fail instead though.)

In this particular case, TestRunner doesn't quote the argument
^[a-zA-Z0-9_.\-]\+$ as it doesn't believe it needs to. The
'[' char in the argument trigger MSYS's argument globbing [1],
which also causes it to interpret the single backslashes as
escapes, making those backslashes vanish.

If we make TestRunner quote arguments if an argument contains a
backslash (which is what the 'not' executable does, with quoting
logic in llvm/lib/Support/Windows/Process.inc), then arguments with
double backslashes end up shrunk to a single backslash (contrary to
regular windows argument quoting rules), breaking a whole bunch of
other tests.

This was the cause of the revert in
f3dd783b239f5587213d528dc642b599f43452b6 <https://reviews.llvm.org/rGf3dd783b239f5587213d528dc642b599f43452b6>.

This allows switching 'not' to a lit internal command, making
TestRunner invoke 'grep' directly in this particular test.

The alternative, to making TestRunner's command line flattening
truly reliable, would be to make TestRunner identify if the target
executable is MSYS based (requiring a PE format inspector in python,
checking if an executable links against msys-2.0.dll or similar),
and apply different quoting rules in those cases.

[1] https://github.com/msys2/msys2-runtime/blob/msys2-3_1_7-release/winsup/cygwin/dcrt0.cc#L208


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99330

Files:
  clang-tools-extra/test/clang-tidy/infrastructure/validate-check-names.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/validate-check-names.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/infrastructure/validate-check-names.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/validate-check-names.cpp
@@ -1,2 +1,6 @@
 // Check names may only contain alphanumeric characters, '-', '_', and '.'.
-// RUN: clang-tidy -checks=* -list-checks | grep '^    ' | cut -b5- | not grep -v '^[a-zA-Z0-9_.\-]\+$'
+// RUN: clang-tidy -checks=* -list-checks | grep '^    ' | cut -b5- | not grep -v '^[a-zA-Z0-9_.\-]*$'
+
+// Hack: The regex should ideally use \+ instead of * to require at least
+// one char, but if grep is a MSYS based tool, the command line argument
+// can end up misparsed by the MSYS runtime.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D99330.333262.patch
Type: text/x-patch
Size: 804 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210325/6b5fdea1/attachment-0001.bin>