[support][llvm-objcopy] Add support for shell wildcards

[rupprecht]

Summary

GNU objcopy accepts the --wildcard flag to allow wildcard matching on symbol-related flags. (Note: it's implicitly true for section flags).

The basic syntax is to allow *, ?, \, and [] which work similarly to how they work in a shell. Additionally, starting a wildcard with ! causes that wildcard to prevent it from matching a flag.

Update GlobPattern in libSupport to handle a few more cases. It does not fully match the fnmatch used by GNU objcopy since named character classes (e.g. [[:digit:]]) are not supported, but this should support most existing use cases (mostly just * is what's used anyway).

[jhenderson]

I've not fully reviewed this yet, and won't get around to it until next week, but I wanted to highlight your own comment from D57517 where you said the following:

Also a question, for others as well: is this a feature we actually want to carry forward? Are there any users of this feature/is it useful? This isn't something I've come across as actually being used, though I'm happy to review it if is.

We added --regex instead of --wildcard precisely to address the specific use-case @evgeny777 had. I'm not opposed to adding --wildcard, if there are actual users of it who don't want to try to migrate to --regex, but I do wonder about the additional complexity this is introducing, and how likely it is to get the rules wrong, e.g. to do with escaping etc.

llvm/lib/Support/Regex.cpp

214 ↗

(On Diff #216686)

Why is BracketDepth signed? It can't ever be negative.

218 ↗

(On Diff #216686)

Looks to me like the comments aren't lined up correctly with their case statements.

255 ↗

(On Diff #216686)

"<=" -> "="

You assert BracketDepth is not negative elsewhere.

[MaskRay]

Also a question, for others as well: is this a feature we actually want to carry forward? Are there any users of this feature/is it useful? This isn't something I've come across as actually being used, though I'm happy to review it if is.

I have the same question.

llvm/lib/Support/Regex.cpp

214 ↗

(On Diff #216686)

BracketDepth should just be a boolean. There is no nested depth. With the current implementation, the pattern [[]* does not match the string [a, while in fnmatch(3) (used by GNU objcopy), it matches.

Windows does not have fnmatch(3). If there were a similar function, Unicode might be its weakness...

To correctly parse brackets used in glob, I think the bracket parsing code should detect [:, [. or [= (
collation-related bracket symbols), and skip all characters until the ending symbol is found.

[rupprecht]

(Will get to the code comments later, just wanted to justify the patch)

In D66613#1642461, @jhenderson wrote:

I've not fully reviewed this yet, and won't get around to it until next week, but I wanted to highlight your own comment from D57517 where you said the following:

Also a question, for others as well: is this a feature we actually want to carry forward? Are there any users of this feature/is it useful? This isn't something I've come across as actually being used, though I'm happy to review it if is.

We added --regex instead of --wildcard precisely to address the specific use-case @evgeny777 had. I'm not opposed to adding --wildcard, if there are actual users of it who don't want to try to migrate to --regex

IMO --regex is nicer for interactive uses, but when using objcopy in a build script that may be used by different toolchains, with either GNU objcopy or llvm-objcopy, a common denominator is needed. Apparently clang+kernel people need it for some versions of the kernel, and we've had some internal users need this too. (For now, they use llvm-objcopy w/ --regex, but it would be a problem if someone using GNU objcopy in their toolchain needs to depend on their package). I also think it will generally help with the long tail of users that want to try it out.

but I do wonder about the additional complexity this is introducing, and how likely it is to get the rules wrong, e.g. to do with escaping etc.

My concern as well. Please review diligently, I won't take comments about regex bugs personally :)

[nickdesaulniers]

In D66613#1642461, @jhenderson wrote:

! In D66613#1642709, @MaskRay wrote:

Also a question, for others as well: is this a feature we actually want to carry forward? Are there any users of this feature/is it useful? This isn't something I've come across as actually being used, though I'm happy to review it if is.

I have the same question.

Android/ChromeOS/Google is looking into using llvm substitutes for binutils for their Linux kernels. This feature is needed for us to substitute GNU strip for llvm-strip and GNU objcopy for llvm-objcopy. The upstream Linux kernel has removed the use of these for architectures we ship, but the Long Term Support (LTS) kernels that we will be shipping for the foreseeable future still do. Backporting the removal of these flags in a way that doesn't also backport features (which is against LTS policy) is likely to be tricky, as it would also cause the LTS branches to start to diverge, making future backports to those source files less likely to apply cleanly. I'm not confident we could get those patches accepted upstream. We could always carry them out of tree in Android/ChromeOS/Google, but I'd rather not.

All that not supporting this feature does is delay the use of llvm-objcopy/llvm-strip in a few major Linux distributions for their kernels for a few years.

[rupprecht]

• Split out separate methods for parsing brackets instead of allowing arbitrary nesting.

[rupprecht]

Here are a couple other places using it in the wild:

https://git.ruby-lang.org/ruby.git/tree/template/Makefile.in#n295
https://chromium.googlesource.com/native_client/pnacl-subzero/+/master/pydir/szbuild.py#441
https://github.com/NetBSD/src/blob/trunk/share/mk/bsd.sys.mk#L352

llvm/lib/Support/Regex.cpp

214 ↗

(On Diff #216686)

Thanks, I missed that edge case.

I've removed the BracketDepth var entirely. I originally did have it as a boolean but it didn't handle the "nested" case of "[[:alpha:]]" (it's not arbitrarily nested, but it is still kind of nested...). It isn't necessary by changing this test case to handle eating all characters until a matching ].

218 ↗

(On Diff #216686)

Fixed -- this indentation level is forced by clang-format, but it looks better after the case statement rather than before it.

[MaskRay]

I just realized that you can remove the glob (I'd call it glob, not wildcard) change from this patch, and just use lib/Support/Regex.cpp:GlobPattern.

GlobPattern is currently used by lld to do version script/dynamic list matching. In version scripts/dynamic lists, [: and [= are syntax error (ld.bfd), and I don't think anyone using [.. But to make it fully fnmatch(pat, str, 0) capable (in case someone uses character classes like [[:digit:]]), you can add these enhancement to a separate change.

llvm/lib/Support/Regex.cpp

247 ↗

(On Diff #216983)

character '^' may be slightly better/faster.

268 ↗

(On Diff #216983)

character ']'

[MaskRay]

llvm/docs/CommandGuide/llvm-objcopy.rst

134 ↗

(On Diff #216983)

On by default for section-related flags.

This is another thing I'm not sure if we want to do.

I'd like users to specify -w to get wildcard semantics for section options. I checked the ruby/nacl/netbsd links and they all appear to use -w

[alexander-shaposhnikov]

khm, I don't insist, but personally I would split out the changes in lib/Support and the corresponding unit tests into a separate patch

[evgeny777]

I wonder if you can use GlobPattern (llvm/Support/GlobPattern.h) or extend this class. One of the reasons we use it in lld is because of much better performance (see D26241),
This may not be the case for llvm-objcopy, but still using original pattern matcher looks nicer than translating to regexps.

[rupprecht]

• Use GlobPattern instead of Regex
• Log a warning if the glob expression is invalid

[rupprecht]

In D66613#1659632, @alexshap wrote:

khm, I don't insist, but personally I would split out the changes in lib/Support and the corresponding unit tests into a separate patch

Ack; I'll submit them separately, but I'm leaving them together in the patch for now, e.g. to show how the added glob support is needed by llvm-objcopy tests.

In D66613#1643926, @MaskRay wrote:

I just realized that you can remove the glob (I'd call it glob, not wildcard) change from this patch, and just use lib/Support/Regex.cpp:GlobPattern.

GlobPattern is currently used by lld to do version script/dynamic list matching. In version scripts/dynamic lists, [: and [= are syntax error (ld.bfd), and I don't think anyone using [.. But to make it fully fnmatch(pat, str, 0) capable (in case someone uses character classes like [[:digit:]]), you can add these enhancement to a separate change.

In D66613#1703550, @evgeny777 wrote:

I wonder if you can use GlobPattern (llvm/Support/GlobPattern.h) or extend this class. One of the reasons we use it in lld is because of much better performance (see D26241),
This may not be the case for llvm-objcopy, but still using original pattern matcher looks nicer than translating to regexps.

OK, removed the regex transformation and using glob instead.

I decided to leave out character classes for now, so the changes to glob are actually pretty minimal. lld tests are still passing w/ these glob changes. I hope the test coverage is sufficient there.

llvm/docs/CommandGuide/llvm-objcopy.rst

134 ↗

(On Diff #216983)

Those were just easy-to-find examples of wildcard usage. There are others using wildcards w/o -w, e.g.

https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/master/Makefile.rules#66

cmd_ec_elf_to_flat ?= $(OBJCOPY) --set-section-flags .roshared=share -R .dram* \
				-O binary $< $@
cmd_ec_elf_to_flat_dram ?= $(OBJCOPY) -j .dram* -O binary $< $@

Older kernel versions: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/firmware/efi/libstub/Makefile?h=linux-4.4.y#n57

STUBCOPY_FLAGS-y		:= -R .debug* -R *ksymtab* -R *kcrctab*

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test

1 ↗

(On Diff #224709)

Move the file-level comment before RUN:

3 ↗

(On Diff #224709)

The name is wildcard-syntax.test (good). Both wildcard and glob are referred to in this test. Personally I think wildcard is a subset of glob and glob is a better name here - glob is a POSIX specified concept and the function fnmatch that objcopy uses accepts glob, not just wildcard.

Unify the naming here.

21 ↗

(On Diff #224709)

What does --remove-section='.???' --remove-section='!.f*' --remove-section='.???' do?

25 ↗

(On Diff #224709)

Full stop.

26 ↗

(On Diff #224709)

If llvm-objcopy called setlocale, RE Bracket Expression (also used by glob) would be non-portable (behaviors of locales other than the POSIX locale are unspecified).

Just for fun, glibc>=2.27 make w and v have the same collating order and a w test may fail in the Swedish locale (if regex matching is used): https://sourceware.org/bugzilla/show_bug.cgi?id=23393 (Comment #41 said this is basically a wontfix)

In any case, llvm-objcopy does not call setlocale, w not used in the test, nor do we use regex, so we are good.

73 ↗

(On Diff #224709)

Probably add a section named z to enhance the test.

llvm/tools/llvm-objcopy/CopyConfig.h

115 ↗

(On Diff #224709)

If ErrorCallback is not stored, consider llvm::function_ref.

119 ↗

(On Diff #224709)

Remove some return

[rupprecht]

• Remove returns that don't compile
• std::function -> llvm::function_ref
• Update docs
• Fix punctuation
• Clarify wildcard vs glob
• Use better check prefixes
• Change letters to avoid any locale issues

[rupprecht]

llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test

3 ↗

(On Diff #224709)

Done; I've kept it as "wildcard" when talking about the flag name, and also once to mention "shell wildcard" which it's commonly referred to as, but generally I've updated to "glob syntax"

21 ↗

(On Diff #224709)

Flag ordering doesn't matter, the algorithm (which GNU objcopy seems to do as well) is to apply the flag if any positive matcher matches and no negative matcher matches. Your example is equivalent to what is written here.

I added some test cases that show this, and I updated the command guide docs.

26 ↗

(On Diff #224709)

Not going to pretend I totally understand locales, but I'll just use [q-s] (to match the r in bar) to avoid v or w anyway :)

73 ↗

(On Diff #224709)

I'm not sure it would add that much, but I added it anyway.

The point of this test is that []xyz] is interpreted as "a single character which is one of something in ]xyz" (which matches the ] section), as opposed to "an empty character matcher followed by the literal xyz]".

I added this as a test comment. Also added a section named xyz] to show that it is not removed.

llvm/tools/llvm-objcopy/CopyConfig.h

119 ↗

(On Diff #224709)

I was wondering how this even compiled, then realized it doesn't :(

[MaskRay]

If you have extra bandwidth, I hope you can raise the issues for projects that use glob in objcopy --*-section but does not pass -w. This patch LG.

[rupprecht]

Turns out treating \ breaks linker scripts on Windows due to path names :(

glob support added in r375051 and immediately reverted in r375052 after a fast buildbot.

[rupprecht]

Planning to reland as-is after D69074

[jhenderson]

Post-commit comments since I've been away and spotted a few relatively minor things. I'll post directly on the commit for the llvm-objcopy side, if I see anything there.

llvm/docs/CommandGuide/llvm-objcopy.rst

135 ↗

(On Diff #216983)

--regex -> :option:'--regex' (with back-ticks instead of apostrophes).

149 ↗

(On Diff #216983)

'!' -> `!` for consistency? Not sure.

llvm/docs/CommandGuide/llvm-strip.rst

109 ↗

(On Diff #216983)

Same comments as llvm-objcopy document.

llvm/lib/Support/GlobPattern.cpp

118

Nit: "* -> '*' ?

llvm/unittests/Support/GlobPatternTest.cpp

26

This and similar cases should at least be ASSERT_TRUE instead of EXPECT_TRUE. If for some reason a failure is hit, the following lines will attempt to dereference an Expected in a failing state and crash the test rather than simply fail it.

Better however would be to use ASSERT_THAT_EXPECTED(Pat1, Succeeded()); (double-check exact usage, but it's something like that).

124

Similar to above, you can use ASSERT_THAT_EXPECTED(Pat2, Failed()); to avoid weird behaviour.

llvm/include/llvm/Support/GlobPattern.h

 
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include <vector>
 
 // This class represents a glob pattern. Supported metacharacters
-// are "*", "?", "[<chars>]" and "[^<chars>]".
+// are "*", "?", "\", "[<chars>]", "[^<chars>]", and "[!<chars>]".
 namespace llvm {
 class BitVector;
 template <typename T> class ArrayRef;
 
 class GlobPattern {
 public:
   static Expected<GlobPattern> create(StringRef Pat);
   bool match(StringRef S) const;

llvm/lib/Support/GlobPattern.cpp

 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 
 using namespace llvm;
 
 static bool hasWildcard(StringRef S) {
-  return S.find_first_of("?*[") != StringRef::npos;
+  return S.find_first_of("?*[\\") != StringRef::npos;
 }
 
 // Expands character ranges and returns a bitmap.
 // For example, "a-cf-hz" is expanded to "abcfghz".
 static Expected<BitVector> expand(StringRef S, StringRef Original) {
   BitVector BV(256, false);
 
   // Expand X-Y.
static Expected<BitVector> expand(StringRef S, StringRef Original) {
   }
 
   for (char C : S)
     BV[(uint8_t)C] = true;
   return BV;
 }
 
 // This is a scanner for the glob pattern.
-// A glob pattern token is one of "*", "?", "[<chars>]", "[^<chars>]"
-// (which is a negative form of "[<chars>]"), or a non-meta character.
+// A glob pattern token is one of "*", "?", "\", "[<chars>]", "[^<chars>]"
+// (which is a negative form of "[<chars>]"), "[!<chars>]" (which is
+// equivalent to "[^<chars>]"), or a non-meta character.
 // This function returns the first token in S.
 static Expected<BitVector> scan(StringRef &S, StringRef Original) {
   switch (S[0]) {
   case '*':
     S = S.substr(1);
     // '*' is represented by an empty bitvector.
     // All other bitvectors are 256-bit long.
     return BitVector();
   case '?':
     S = S.substr(1);
     return BitVector(256, true);
   case '[': {
-    size_t End = S.find(']', 1);
+    // ']' is allowed as the first character of a character class. '[]' is
+    // invalid. So, just skip the first character.
+    size_t End = S.find(']', 2);
     if (End == StringRef::npos)
       return make_error<StringError>("invalid glob pattern: " + Original,
                                      errc::invalid_argument);
 
     StringRef Chars = S.substr(1, End - 1);
     S = S.substr(End + 1);
-    if (Chars.startswith("^")) {
+    if (Chars.startswith("^") || Chars.startswith("!")) {
       Expected<BitVector> BV = expand(Chars.substr(1), Original);
       if (!BV)
         return BV.takeError();
       return BV->flip();
     }
     return expand(Chars, Original);
   }
+  case '\\':
+    // Eat this character and fall through below to treat it like a non-meta
+    // character.
+    S = S.substr(1);
+    LLVM_FALLTHROUGH;
   default:
     BitVector BV(256, false);
     BV[(uint8_t)S[0]] = true;
     S = S.substr(1);
     return BV;
   }
 }
 
 Expected<GlobPattern> GlobPattern::create(StringRef S) {
   GlobPattern Pat;
 
   // S doesn't contain any metacharacter,
   // so the regular string comparison should work.
   if (!hasWildcard(S)) {
     Pat.Exact = S;
     return Pat;
   }
 
-  // S is something like "foo*". We can use startswith().
-  if (S.endswith("*") && !hasWildcard(S.drop_back())) {
+  // S is something like "foo*", and the "* is not escaped. We can use
+  // startswith().
+  if (S.endswith("*") && !S.endswith("\\*") && !hasWildcard(S.drop_back())) {
     Pat.Prefix = S.drop_back();
     return Pat;
   }
 
   // S is something like "*foo". We can use endswith().
   if (S.startswith("*") && !hasWildcard(S.drop_front())) {
     Pat.Suffix = S.drop_front();
     return Pat;

llvm/unittests/Support/GlobPatternTest.cpp

 //===- llvm/unittest/Support/GlobPatternTest.cpp --------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/GlobPattern.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 namespace {
 
 class GlobPatternTest : public ::testing::Test {};
 
-TEST_F(GlobPatternTest, Basics) {
+TEST_F(GlobPatternTest, Empty) {
   Expected<GlobPattern> Pat1 = GlobPattern::create("");
   EXPECT_TRUE((bool)Pat1);
   EXPECT_TRUE(Pat1->match(""));
   EXPECT_FALSE(Pat1->match("a"));
+}
+
+TEST_F(GlobPatternTest, Glob) {
+  Expected<GlobPattern> Pat1 = GlobPattern::create("ab*c*def");
+  EXPECT_TRUE((bool)Pat1);
+  EXPECT_TRUE(Pat1->match("abcdef"));
+  EXPECT_TRUE(Pat1->match("abxcxdef"));
+  EXPECT_FALSE(Pat1->match(""));
+  EXPECT_FALSE(Pat1->match("xabcdef"));
+  EXPECT_FALSE(Pat1->match("abcdefx"));
+}
+
+TEST_F(GlobPatternTest, Wildcard) {
+  Expected<GlobPattern> Pat1 = GlobPattern::create("a??c");
+  EXPECT_TRUE((bool)Pat1);
+  EXPECT_TRUE(Pat1->match("axxc"));
+  EXPECT_FALSE(Pat1->match("axxx"));
+  EXPECT_FALSE(Pat1->match(""));
+}
+
+TEST_F(GlobPatternTest, Escape) {
+  Expected<GlobPattern> Pat1 = GlobPattern::create("\\*");
+  EXPECT_TRUE((bool)Pat1);
+  EXPECT_TRUE(Pat1->match("*"));
+  EXPECT_FALSE(Pat1->match("\\*"));
+  EXPECT_FALSE(Pat1->match("a"));
+
+  Expected<GlobPattern> Pat2 = GlobPattern::create("a?\\?c");
+  EXPECT_TRUE((bool)Pat2);
+  EXPECT_TRUE(Pat2->match("ax?c"));
+  EXPECT_FALSE(Pat2->match("axxc"));
+  EXPECT_FALSE(Pat2->match(""));
+}
+
+TEST_F(GlobPatternTest, BasicCharacterClass) {
+  Expected<GlobPattern> Pat1 = GlobPattern::create("[abc-fy-z]");
+  EXPECT_TRUE((bool)Pat1);
+  EXPECT_TRUE(Pat1->match("a"));
+  EXPECT_TRUE(Pat1->match("b"));
+  EXPECT_TRUE(Pat1->match("c"));
+  EXPECT_TRUE(Pat1->match("d"));
+  EXPECT_TRUE(Pat1->match("e"));
+  EXPECT_TRUE(Pat1->match("f"));
+  EXPECT_TRUE(Pat1->match("y"));
+  EXPECT_TRUE(Pat1->match("z"));
+  EXPECT_FALSE(Pat1->match("g"));
+  EXPECT_FALSE(Pat1->match(""));
+}
+
+TEST_F(GlobPatternTest, NegatedCharacterClass) {
+  Expected<GlobPattern> Pat1 = GlobPattern::create("[^abc-fy-z]");
+  EXPECT_TRUE((bool)Pat1);
+  EXPECT_TRUE(Pat1->match("g"));
+  EXPECT_FALSE(Pat1->match("a"));
+  EXPECT_FALSE(Pat1->match("b"));
+  EXPECT_FALSE(Pat1->match("c"));
+  EXPECT_FALSE(Pat1->match("d"));
+  EXPECT_FALSE(Pat1->match("e"));
+  EXPECT_FALSE(Pat1->match("f"));
+  EXPECT_FALSE(Pat1->match("y"));
+  EXPECT_FALSE(Pat1->match("z"));
+  EXPECT_FALSE(Pat1->match(""));
 
-  Expected<GlobPattern> Pat2 = GlobPattern::create("ab*c*def");
+  Expected<GlobPattern> Pat2 = GlobPattern::create("[!abc-fy-z]");
   EXPECT_TRUE((bool)Pat2);
-  EXPECT_TRUE(Pat2->match("abcdef"));
-  EXPECT_TRUE(Pat2->match("abxcxdef"));
+  EXPECT_TRUE(Pat2->match("g"));
+  EXPECT_FALSE(Pat2->match("a"));
+  EXPECT_FALSE(Pat2->match("b"));
+  EXPECT_FALSE(Pat2->match("c"));
+  EXPECT_FALSE(Pat2->match("d"));
+  EXPECT_FALSE(Pat2->match("e"));
+  EXPECT_FALSE(Pat2->match("f"));
+  EXPECT_FALSE(Pat2->match("y"));
+  EXPECT_FALSE(Pat2->match("z"));
   EXPECT_FALSE(Pat2->match(""));
-  EXPECT_FALSE(Pat2->match("xabcdef"));
-  EXPECT_FALSE(Pat2->match("abcdefx"));
+}
+
+TEST_F(GlobPatternTest, BracketFrontOfCharacterClass) {
+  Expected<GlobPattern> Pat1 = GlobPattern::create("[]a]x");
+  EXPECT_TRUE((bool)Pat1);
+  EXPECT_TRUE(Pat1->match("]x"));
+  EXPECT_TRUE(Pat1->match("ax"));
+  EXPECT_FALSE(Pat1->match("a]x"));
+  EXPECT_FALSE(Pat1->match(""));
+}
 
-  Expected<GlobPattern> Pat3 = GlobPattern::create("a??c");
-  EXPECT_TRUE((bool)Pat3);
-  EXPECT_TRUE(Pat3->match("axxc"));
-  EXPECT_FALSE(Pat3->match("axxx"));
-  EXPECT_FALSE(Pat3->match(""));
-
-  Expected<GlobPattern> Pat4 = GlobPattern::create("[abc-fy-z]");
-  EXPECT_TRUE((bool)Pat4);
-  EXPECT_TRUE(Pat4->match("a"));
-  EXPECT_TRUE(Pat4->match("b"));
-  EXPECT_TRUE(Pat4->match("c"));
-  EXPECT_TRUE(Pat4->match("d"));
-  EXPECT_TRUE(Pat4->match("e"));
-  EXPECT_TRUE(Pat4->match("f"));
-  EXPECT_TRUE(Pat4->match("y"));
-  EXPECT_TRUE(Pat4->match("z"));
-  EXPECT_FALSE(Pat4->match("g"));
-  EXPECT_FALSE(Pat4->match(""));
-
-  Expected<GlobPattern> Pat5 = GlobPattern::create("[^abc-fy-z]");
-  EXPECT_TRUE((bool)Pat5);
-  EXPECT_TRUE(Pat5->match("g"));
-  EXPECT_FALSE(Pat5->match("a"));
-  EXPECT_FALSE(Pat5->match("b"));
-  EXPECT_FALSE(Pat5->match("c"));
-  EXPECT_FALSE(Pat5->match("d"));
-  EXPECT_FALSE(Pat5->match("e"));
-  EXPECT_FALSE(Pat5->match("f"));
-  EXPECT_FALSE(Pat5->match("y"));
-  EXPECT_FALSE(Pat5->match("z"));
-  EXPECT_FALSE(Pat5->match(""));
+TEST_F(GlobPatternTest, SpecialCharsInCharacterClass) {
+  Expected<GlobPattern> Pat1 = GlobPattern::create("[*?^]");
+  EXPECT_TRUE((bool)Pat1);
+  EXPECT_TRUE(Pat1->match("*"));
+  EXPECT_TRUE(Pat1->match("?"));
+  EXPECT_TRUE(Pat1->match("^"));
+  EXPECT_FALSE(Pat1->match("*?^"));
+  EXPECT_FALSE(Pat1->match(""));
 }
 
 TEST_F(GlobPatternTest, Invalid) {
   Expected<GlobPattern> Pat1 = GlobPattern::create("[");
   EXPECT_FALSE((bool)Pat1);
   handleAllErrors(Pat1.takeError(), [&](ErrorInfoBase &EIB) {});
+
+  Expected<GlobPattern> Pat2 = GlobPattern::create("[]");
+  EXPECT_FALSE((bool)Pat2);
+  handleAllErrors(Pat2.takeError(), [&](ErrorInfoBase &EIB) {});
 }
 
 TEST_F(GlobPatternTest, ExtSym) {
   Expected<GlobPattern> Pat1 = GlobPattern::create("a*\xFF");
   EXPECT_TRUE((bool)Pat1);
   EXPECT_TRUE(Pat1->match("axxx\xFF"));
   Expected<GlobPattern> Pat2 = GlobPattern::create("[\xFF-\xFF]");
   EXPECT_TRUE((bool)Pat2);
   EXPECT_TRUE(Pat2->match("\xFF"));
 }
 }