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

Mon Sep 27 00:29:23 PDT 2021

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

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:4-5
+## When a section is renamed, its relocation section should be renamed similarly.
+# RUN: llvm-objcopy %t - --rename-section=.text=.foo --rename-section=.data=.bar | \
+# RUN:   llvm-readobj --sections - | \
+# RUN:   FileCheck %s --check-prefix=TEST1
----------------
I'm not going to insist on this, but I have a strong preference for writing the output file to disk, and then feeding that file to llvm-readobj. The reason is because it makes test debugging significantly easier (if something fails, you can just go and inspect the file on disk, without having to modify the test to create the object).

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:12-13
+
+## A relocation section should be renamed when its target section is renamed
+## even if its name does not follow the typical pattern.
+## Note that in the input file, '.rel.fn' targets '.sub', not '.fn'.
----------------
No strong opinion on this, so it's fine if you prefer it this way, but did you consider having just one llvm-objcopy execution that tests all the test cases? If you did that, I'd name the sections so that they describe the case they correspond to.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:22-23
+
+## A relocation section should not be renamed if an unrelated section with
+## the same common name is related.
+## Note that in the input file, 'rel.fn' does not target 'fn'.
----------------
I think the final "related" should be "renamed"?

================
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 - | \
----------------
Maybe rename one of these to something that doesn't have ".rel" or ".rela" as a prefix?

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:42-43
+## Explicit renaming is preferred.
+# RUN: llvm-objcopy %t - --rename-section=.text=.foo --rename-section=.data=.bar --rename-section=.sub=.func \
+# RUN:     --rename-section=.rel.text=.rel.baz --rename-section=.rela.data=.rela.qux --rename-section=.rel.fn=.funcrelocs | \
+# RUN:   llvm-readobj --sections - | \
----------------
This might just be because of my fairly small laptop screen, but it might be better if this was split over a third line, to reduce the width.

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

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:63
+
+## A dynamic relocation section should not be renamed together with its target section
+# RUN: llvm-objcopy %t - --rename-section=.got.plt=.foo | \
----------------

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:79
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
----------------
There's quite a big gap between the key and value, without it adding anything. I think here and below you should remove the excessive spacing - it makes things a little easier for me to read, I find. The approach I follow, is to keep things vertically lined up within a block, such that there is one space between the longest key and its value.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test:86
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+  - Name:            .rel.text
----------------
Here and below, with the execption of the .rela.plt section, I don't think the flags are relevant, so I'd remove them to remove noise from the patch.

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

================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:611
+    }
+    // Rename relocation sections according to their target sections.
+    for (RelocationSectionBase *RelocSec : RelocSections) {
----------------
Nit: I'd be tempted to put a blank line before this comment.

================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:613
+    for (RelocationSectionBase *RelocSec : RelocSections) {
+      auto Iter = RenamedSections.find(RelocSec->getSection());
+      if (Iter != RenamedSections.end())
----------------
What happens if the sh_link field of the relocation section is 0/invalid? Will this blow up?

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

https://reviews.llvm.org/D110352