[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/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:
> 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.
Yes. This flag renames dynamic relocation sections too. It seems that GNU objcopy simply checks whether ALLOC flag is set: 

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=binutils/objcopy.c;h=330b93c1ad3f87a0f5777ac88dc4af1398959bbe;hb=HEAD#l3772


================
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();
----------------
jakehehrlich wrote:
> 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.
> 
> 
I think that's not sufficient. If both --rename-section and --prefix-alloc-sections are given, GNU objcopy renames sections by  --rename-section first and then adds the prefix specified by --prefix-alloc-sections.

For example, `--rename-section=.text=.text2 --prefix-alloc-sections=.prefix` renames .rel.text to .rel.prefix.text2.

It looks that the current patch only renames correctly if the target section comes //before// its relocation section by the way.

I have two ideas:
- Iterate sections twice: rename sections first and then add the prefix.
- Add `Renamed` flag to `SectionBase`: check the flag here and rename the section if needed.

I would like to hear your thoughts on this @jakehehrlich .


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

https://reviews.llvm.org/D60042