[PATCH] D110352: [llvm-objcopy] Rename relocation sections together with their targets.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:11
+# RUN:    --rename-section=.tst7.rela.plt=.tst7.ren.rela.plt
+# RUN: llvm-readobj --sections %t1 | FileCheck %s
+
----------------
Add something to prevent prefix-only matches with FileCheck. At the moment, a name ".rel.tst1.ren.foo.bar" would match the second CHECK line in the first test case, which isn't the intent. I'd suggest either adding `--match-full-lines` to the FileCheck run, or adding `{{ }}` (or `{{$}}` if it's the line end - can't remember if there's anything after the name) to the end of each name.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --rename-section=.text=.text2 --rename-section=.data=.data2 %t %t2
----------------
ikudrin wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > ikudrin wrote:
> > > > MaskRay wrote:
> > > > > The two tests can be combined.
> > > > > 
> > > > > One file with different cases is sometimes easier to comprehend than having multiple files.
> > > > > 
> > > > > Is there a test renaming non-SHF_ALLOC relocation section?
> > > > > Is there a test renaming non-SHF_ALLOC relocation section?
> > > > 
> > > > There are no tests for these cases because I didn't have the intention to fasten the behavior. Previously, `llvm-objcopy` allowed such sections to be renamed with an explicit `--rename-section`. GNU `objcopy` and the patched `llvm-objcopy` ignore the setting. If someone ever finds that important, they should be free to improve the implementation in whatever way.
> > > I have some disagreements about the behaviour in this patch (see out-of-line comment), but if we do stick with the behaviour you've proposed, I think we should test some additional cases:
> > > 1) Dynamic relocations are not implicitly renamed when their target sections are.
> > > 2) Non-dynamic relocation sections which target a section with an unrelated name, are renamed if their target section is renamed (i.e. show the behaviour is driven by the target section link, not some link based on name).
> > > 3) Non-dynamic relocaction sections which target a section with an unrelated name, are not renamed if the target section is not renamed.
> > > 4) Non-dynamic relocation sections which target a section with a different name, but which have a common name with an unrelated section, are not renamed if the unrelated section is renamed (i.e. again show that the sh_info value is what's important, not the name).
> > > Some of these points are applicable even if we adopt the behaviour I'm suggesting in my out-of-line comment.
> > One more test case, I think we need is - a relocation section whose prefix doesn't match its type (or indeed which doesn't have a prefix), so e.g. ".rela.foo" is a SHT_REL section, and ".baz" is a SHT_RELA section etc.
> Please, take a look at `Test 2` in the updated test file, where the relocation section is called `.tst2.foo.rel`. Does that match your suggestion?
Thanks, yes that works for me.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:613
+    for (RelocationSectionBase *RelocSec : RelocSections) {
+      auto Iter = RenamedSections.find(RelocSec->getSection());
+      if (Iter != RenamedSections.end())
----------------
ikudrin wrote:
> jhenderson wrote:
> > What happens if the sh_link field of the relocation section is 0/invalid? Will this blow up?
> You probably meant `sh_info`, not `sh_link`? In that case, `getSection()` will just return `nullptr`, which is not in the set, so `find()` will return `RenamedSections.end()`.
Yeah, I meant sh_info, thanks.


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

https://reviews.llvm.org/D110352