[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:
> > 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.
> We have a test case which renames dynamic relocation sections. Should I clarify that in a comment or add a test case against a binary file, say, `test/tools/llvm-objcopy/ELF/Inputs/dynrel.elf`?
Those two cases are slightly different and reflects more realistic set up than the yaml file does. Should be a short 5 or so line test.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:629
+            // section comes *before* the target section, we add the prefix.
+            if (PrefixedSections.find(TargetSec->Name) !=
+                PrefixedSections.end()) {
----------------
I'm *really* against doing this kind of string searching when we can build a map. What if a section already contains the prefix being used. This won't be consistent in that case.

Also you should add a test case for that above.


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

https://reviews.llvm.org/D60042