[PATCH] D57517: [llvm-objcopy] Allow using regex in name comparison

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked an inline comment as done.
rupprecht added a comment.

In D57517#1380215 <https://reviews.llvm.org/D57517#1380215>, @jhenderson wrote:

> In D57517#1380163 <https://reviews.llvm.org/D57517#1380163>, @evgeny777 wrote:
>
> > > Are there any users of this feature/is it useful?
> >
> > One of such users is myself. I'm using it to strip specific classes (with all methods and data) from relocatable objects and then execute those object files on memory tight embedded platform using OrcJIT. This is pretty specific use case though.
>
>
> Still sounds like a reasonable use-case anyway. I could see it being useful for library providers too, who want to hide some things in the external interface, though I don't know of any concrete examples.
>
> If we choose to support this (and I'm happy to +1 it), then I think it should only be a long-form switch, with no short-form equivalent. That's because there's a much higher chance of something being added to GNU objcopy in the future with a short alias that causes a clash than for a long-form version.


OK, thanks -- just wanted to double check it's useful, and sounds like it is. I'm on board too :)



================
Comment at: test/tools/llvm-objcopy/ELF/globalize.test:7
 # RUN: llvm-readobj --symbols %t2 | FileCheck %s
+# RUN: llvm-objcopy -regex --globalize-symbol '[.]*' %t %t3
+# RUN: llvm-readobj --symbols %t3 | FileCheck %s
----------------
nit: please use --regex instead of -regex in these test cases


================
Comment at: test/tools/llvm-objcopy/ELF/globalize.test:8
+# RUN: llvm-objcopy -regex --globalize-symbol '[.]*' %t %t3
+# RUN: llvm-readobj --symbols %t3 | FileCheck %s
 
----------------
In cases where this should be identical to the output of another another command, you can use `cmp` instead of another `readobj | FileCheck`, e.g. `cmp %t2 %t3`


================
Comment at: tools/llvm-objcopy/CopyConfig.h:52
 
+template <class NameTy> class NameOrRegex {
+  NameTy Name;
----------------
Can you add a test that just demonstrates regex matching itself?


================
Comment at: tools/llvm-objcopy/CopyConfig.h:64
+  }
+  bool operator==(StringRef S) const { return R ? R->match(S) : Name == S; }
+  bool operator!=(StringRef S) const { return !operator==(S); }
----------------
It looks like this does regex matching anywhere within the string. I think it might be less error prone if we make it a full string regex match, e.g. `special.*` -- currently matches `not-special`. I think we should force the user to write `.*special.*` if that's what they want.

Python has a similar distinction, re.match vs re.search. I don't think llvm's regex has anything similar builtin, so you may need to manually wrap the expression in ^$.


================
Comment at: tools/llvm-objcopy/CopyConfig.h:101
+  std::vector<NameOrRegex<StringRef>> ToRemove;
+  std::vector<NameOrRegex<std::string>> SymbolsToKeepGlobal;
 
----------------
It might be worth using StringSaver or something for this, so they can all be StringRef and avoid templating NameOrRegex. I'll take a look since I introduced that nonsense here.


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

https://reviews.llvm.org/D57517