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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
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
----------------
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?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:33
+## A relocation section can be renamed with an explicit --rename-section command.
+# RUN: llvm-objcopy %t - --rename-section=.rel.text=.rel.foo --rename-section=.rela.data=.rela.bar | \
+# RUN:   llvm-readobj --sections - | \
----------------
jhenderson wrote:
> Maybe rename one of these to something that doesn't have ".rel" or ".rela" as a prefix?
Done in the `Test 4` in the update test file. `.rel.tst4.foo` is renamed to `.tst4.ren.foo.rel`.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:54-55
+
+## Should not rename a relocation section if it or its target are not specified in --rename-section.
+## Note that this diverges from GNU objcopy.
+# RUN: llvm-objcopy %t - --rename-section=.text=.foo | \
----------------
jhenderson wrote:
> By extending one of the other checks, you can avoid adding another llvm-objcopy/llvm-readobj execution, since this case is also exercised by the invocation for e.g. TEST1.
In the updated test file, there are no `--rename-section` commands for `.tst5` sections.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:604-608
+        // their explicit '--rename-section' commands untile after their target
+        // sections are renamed,
+        // Dynamic relocation sections (i.e. ones with SHF_ALLOC) should be
+        // renamed only explicitly. Otherwise, renaming, for example, 'got.plt'
+        // would affect '.rela.plt', which is not desirable.
----------------
jhenderson wrote:
> 
Thanks! I have to be more attentive...


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:613
+    for (RelocationSectionBase *RelocSec : RelocSections) {
+      auto Iter = RenamedSections.find(RelocSec->getSection());
+      if (Iter != RenamedSections.end())
----------------
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()`.


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

https://reviews.llvm.org/D110352