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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

The llvm-objcopy design goal was to be behave like GNU objcopy, as far as GNU objcopy's behaviour made sense. I think renaming relocation sections like this implicitly does indeed make sense. However, I don't think think the dynamic/non-dynamic distinction makes much sense to me. I personally think the behaviour should be:

1. If a section is renamed, a relocation section targeting it will be renamed, if not explicitly named in another --rename-section command.
2. If a relocation section is specified by --rename-section, it will be renamed accordingly.
3. Relocation sections whose targets are not renamed are not renamed themselves, even if the names are already different (this ensures changes via point 2 above aren't lost).



================
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:
> 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.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:6-9
+## Check that when renaming a section then the corresponding relocation section
+## is renamed similarly.
+## A dynamic relocation section can be renamed with an explicit
+## --rename-section. That resembles the behavior of GNU objcopy.
----------------
This wording sounds a bit more natural to me.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:612
     }
+    // Rename relocation sections according to their targets.
+    for (RelocationSectionBase *RelocSec : RelocSections) {
----------------
I think this could be a little more explicit, and avoid ambiguity about "targets"


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

https://reviews.llvm.org/D110352