[PATCH] D58960: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/compress-debug-sections-relocations.test:1
+# REQUIRES: zlib
+
----------------
Could you reuse the Inputs/compress-debug-sections.yaml input file? Does that cover enough here, if updated with an actual symbol (see the update to that file in D58510)? I vaguely remember requesting that the relocation existed, so that it would test relocation behaviour, but it turns out the yaml was not quite correct.


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:257
 
-    for (RelocationSection *RS : RelocationSections) {
-      if (RS->getSection() == S)
-        RS->setSection(NewSection);
-    }
-  }
+  // Now we want to update target sections of relocation sections.
+  // Also we will update the relocations itself.
----------------
"update target" -> "update the target"


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:258
+  // Now we want to update target sections of relocation sections.
+  // Also we will update the relocations itself.
+  for (auto &Sec : Obj.sections())
----------------
itself -> themselves.

Perhaps also worth mentioning how you are updating them (i.e. updating the symbol references).


================
Comment at: tools/llvm-objcopy/ELF/Object.h:599
   void markSymbols() override;
+  void replaceSections(const DenseMap<SectionBase *, SectionBase *> &FromTo);
 
----------------
A more in-keeping with the llvm-objcopy Object design might be to provide a do-nothing virtual function for all sections "replaceSectionReferences" or similar, and then override it for section types that actually need some work, like relocation sections.

In particular, I could imagine some issues with things like group sections referencing debug data or similar.


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

https://reviews.llvm.org/D58960