[PATCH] D66613: [support][llvm-objcopy] Add support for shell wildcards

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test:3
+
+## This test checks that llvm-objcopy accepts wildcard syntax correctly.
+
----------------
MaskRay wrote:
> 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.
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"


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test:21
+## ! (as a leading character) prevents matches (not dependent on ordering).
+# RUN: llvm-objcopy --remove-section='.???' --remove-section='!.f*' %t.o %t.negmatch.o
+# RUN: llvm-readobj --sections %t.negmatch.o \
----------------
MaskRay wrote:
> What does `--remove-section='.???' --remove-section='!.f*' --remove-section='.???'` do?
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.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test:26
+## [a-z] matches a range of characters
+# RUN: llvm-objcopy --remove-section='.[a-c][a-a][q-z]' %t.o %t.range.o
+# RUN: llvm-readobj --sections %t.range.o \
----------------
MaskRay wrote:
> 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.
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 :)


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test:73
+## ] doesn't close the character class as a first character.
+# RUN: llvm-objcopy --remove-section='[]xyz]' %t.special.o %t.class.2.o
+# RUN: llvm-readobj --sections %t.class.2.o \
----------------
MaskRay wrote:
> Probably add a section named `z` to enhance the test.
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.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:119
+  bool operator==(StringRef S) const {
+    return R ? return R->match(S) : G ? return G->match(S) : Name == S;
+  }
----------------
MaskRay wrote:
> Remove some `return`
I was wondering how this even compiled, then realized it doesn't :(


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66613/new/

https://reviews.llvm.org/D66613