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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

A few comments.



================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:607
+        // .rela.prefix.plt since GNU objcopy does so.
+        } else if (isa<RelocationSectionBase>(&Sec)) {
+          auto *TargetSec = dyn_cast<RelocationSectionBase>(&Sec)->getSection();
----------------
You can do `if (auto *RelocSec = dyn_cast<RelocationSectionBase>(&Sec))` and then `SectionBase *TargetSec = RelocSec->getSection()`


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:607
+        // .rela.prefix.plt since GNU objcopy does so.
+        } else if (isa<RelocationSectionBase>(&Sec)) {
+          auto *TargetSec = dyn_cast<RelocationSectionBase>(&Sec)->getSection();
----------------
jakehehrlich wrote:
> You can do `if (auto *RelocSec = dyn_cast<RelocationSectionBase>(&Sec))` and then `SectionBase *TargetSec = RelocSec->getSection()`
I forget. Does this apply to dynamic relocation sections as well? Does this flag rename the dynamic relocation section? My hunch is that it doesn't.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:626
+            // section comes *before* the target section, we add the prefix.
+            if (TargetSec->Name.find(Config.AllocSectionsPrefix) == 0)
+              Sec.Name = (prefix + TargetSec->Name).str();
----------------
This is not a good way to do this.

I think the right way is to have the "OriginalName" and do this in a non-conditional way. I tried thinking about a bunch of alternatives but they all have sticky issues. Your best bet is to just add OriginalName to SectionBase and unconditionally assign the section name here.




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

https://reviews.llvm.org/D60042