[PATCH] D60042: [llvm-objcopy] Add --prefix-alloc-sections

[Seiya Nuta via Phabricator via llvm-commits]

seiya marked 2 inline comments as done.
seiya added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-and-prefix-alloc-sections.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --rename-section=.text=.text2 --rename-section=.data=.data2 --prefix-alloc-sections=.prefix %t %t2
----------------
jhenderson wrote:
> In this test, you don't really want to test the cases that you already tested in the main --prefix-alloc-sections test. As a result, you can probably remove the bar section, at the very least, nor do you need to test a section that isn't renamed at all.
> 
> I'm also not sure you get anything by testing two different section renames.
Indeed. I'll remove the test.

> I'm also not sure you get anything by testing two different section renames.
We should test the cases when the relocation section comes after/before its target section because this patch deals with them differently to perform both `--rename-section` and `--prefix-alloc-sections` in one pass.

 I'll update the patch to mention it in this test file.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:633
+              const auto Iter = Config.SectionsToRename.find(TargetSec->Name);
+              if (Iter != Config.SectionsToRename.end()) {
+                // Both `--rename-section` and `--prefix-alloc-sections` are
----------------
I mean this if block.


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

https://reviews.llvm.org/D60042