[PATCH] D46369: [llvm-objcopy] Add --strip-symbol (-N) option

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:910
 
+void Object::removeSymbols(std::function<bool(const Symbol &)> ToRemove) {
+  std::vector<const SectionBase *> Groups;
----------------
I'd like to move everything over to using function_ref where possible. The way we accumulate in HandleArgs will remain std::function but I think preety much everywhere else we can use function_ref.


================
Comment at: tools/llvm-objcopy/Object.cpp:919
+
+  if (!SymbolTable)
+    return;
----------------
If this is still needed in the final version of this method, make sure it comes before any pre-computation.


================
Comment at: tools/llvm-objcopy/Object.cpp:925
+    if (ToRemove(Sym)) {
+      const auto &It = std::find_if(std::begin(Groups), std::end(Groups),
+                                    [&Sym, this](const SectionBase *Sec) {
----------------
This is the wrong way to do this.  Link and info are not the only way to reference symbols and you can't know how those sections are using Link/Info here so you might get false positives. This is also unacceptably inefficient. For instance we support chromium and chromium uses --ffunction-sections. That would make this an n^2 loop where n = about 60k (also n will grow to over 100k after I implement large section indexes properly. They're currently using tricks to only run llvm-objcopy on fewer sections at a time.)  It's actually worse than n^2 because data symbols are also present. So this is unacceptably inefficient as is. We need a linear time algorithm for removing symbols. In general I want to be linear with respect to number of symbols and n*lg(n) with respect to sections (and I'd like to push for linear in the case where no program headers exist but I don't think that's the case right now).

I didn't think about this at the time but its slightly tricky to get this efficient. I think instead of using 'removeSymbolReferences' giving every section a 'removeSymbols' method that takes a predicate might be called for. The idea is that each section can a) ignore b) throw an error or c) actually remove symbols.

Then you're code is a loop over sections, one of which contains a loop over symbols. You also don't need the extra loop over sections to get the group sections.


================
Comment at: tools/llvm-objcopy/Object.cpp:930
+                                    });
+      if (It != std::end(Groups))
+        removeSections([&It](const SectionBase &SecBase) {
----------------
Actually for this patch, I'd prefer we not remove groups and instead throw an error explaining that you could instead just remove those group sections if you really want that. I don't *think* anyone is relaying on llvm-objcopy to remove those sections when that symbol is removed. If they are they can file a bug report.


Repository:
  rL LLVM

https://reviews.llvm.org/D46369