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.
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.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
