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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich 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();
----------------
seiya wrote:
> 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
Can you add a test case for this. We have a binary checked in that you can use. I want to make sure it isn't double renamed and all that. It's just a slightly more tricky real world use case.


================
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();
----------------
seiya wrote:
> seiya wrote:
> > 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 .
> Or add `OriginalName` to SectionBase and use `Config.SectionsToRename` here:
> ```lang=cpp
>   const auto Iter = Config.SectionsToRename.find(TargetSec->OriginalName);
>   if (Iter != Config.SectionsToRename.end()) {
>     // Both `--rename-section` and `--prefix-alloc-sections` are given.
>     const SectionRename &SR = Iter->second;
>     Sec.Name = (prefix + Config.AllocSectionsPrefix + SR.NewName).str();
>   } else {
>     Sec.Name = (prefix + Config.AllocSectionsPrefix + TargetSec->Name).str();
>   }
> ```
Ah right, good catch. I think the proposal you just gave is better than what we currently have but the issue of the section being renamed should probably happen more smoothly I suppose. In principal we could add a new mechanism for section renaming in the future which would invalidate this and then require us to update it in the future.

What if you build a map of renamed sections here and then check if the section is renamed. That should avoid the fragility of checking for the prefix like this.


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

https://reviews.llvm.org/D60042