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

[ikudrin]

Summary

As for now, llvm-objcopy renames only sections that are specified explicitly in --rename-section, while GNU objcopy keeps names of relocation sections in sync with their targets. For example:

> readelf -S test.o
...
  [ 1] .foo      PROGBITS
  [ 2] .rela.foo RELA

> objcopy --rename-section .foo=.bar test.o gnu.o
> readelf -S gnu.o
...
  [ 1] .bar      PROGBITS
  [ 2] .rela.bar RELA

> llvm-objcopy --rename-section .foo=.bar test.o llvm.o
> readelf -S llvm.o
...
  [ 1] .bar      PROGBITS
  [ 2] .rela.foo RELA

This patch makes llvm-objcopy to match the behavior of GNU objcopy better.

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test

2

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?

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

596

You can omit SHT_REL[A] since the list will be incomplete. We may have [[ https://sourceware.org/bugzilla/show_bug.cgi?id=27924 | SHT_RELR ]] in the future :)

617–618

according to?

[ikudrin]

Thanks for the review!

• Combined tests
• Updated comments

llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test

2

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.

[jhenderson]

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

llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test

2

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.

7–10

This wording sounds a bit more natural to me.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

618

I think this could be a little more explicit, and avoid ambiguity about "targets"

[ikudrin]

Thanks for the suggestions!

I've tried to integrate all of them, except for ignoring SHF_ALLOC. We should not automatically rename dynamic relocation sections because their names may not follow the typical pattern, e.g. .rela.plt targets .got.plt, and renaming them together seems wrong.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test

2

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.

5–6

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

13–14

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.

23–24

I think the final "related" should be "renamed"?

34

Maybe rename one of these to something that doesn't have ".rel" or ".rela" as a prefix?

43–44

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.

55–56

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.

64

80

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.

87

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.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

604–608

611

Nit: I'd be tempted to put a blank line before this comment.

613

What happens if the sh_link field of the relocation section is 0/invalid? Will this blow up?

[ikudrin]

Thank you for the review, @jhenderson!

• The test is rewritten so that llvm-objcopy runs only once.
• An intermediate file is kept.

llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test

2

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?

34

Done in the Test 4 in the update test file. .rel.tst4.foo is renamed to .tst4.ren.foo.rel.

55–56

In the updated test file, there are no --rename-section commands for .tst5 sections.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

604–608

Thanks! I have to be more attentive...

613

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().

[ikudrin]

• Fix a formatting issue

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test

2

Thanks, yes that works for me.

12

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.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

613

Yeah, I meant sh_info, thanks.

[ikudrin]

• Add {{ }} in checks

[jhenderson]

LGTM!

llvm/test/tools/llvm-objcopy/ELF/rename-section-relocsec.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy %t %t1 \
+# RUN:    --rename-section=.tst1.foo=.tst1.ren.foo \
+# RUN:    --rename-section=.tst1.bar=.tst1.ren.bar \
+# RUN:    --rename-section=.tst2.foo=.tst2.ren.foo \
+# RUN:    --rename-section=.tst3.foo=.tst3.ren.foo \
+# RUN:    --rename-section=.tst4.foo=.tst4.ren.foo \
+# RUN:    --rename-section=.rel.tst4.foo=.tst4.ren.foo.rel \
+# RUN:    --rename-section=.tst6.got.plt=.tst6.ren.got.plt \
+# RUN:    --rename-section=.tst7.rela.plt=.tst7.ren.rela.plt
+# RUN: llvm-readobj --sections %t1 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+
+## Test 1. When a section is renamed, its relocation section should be renamed
+## similarly.
+# CHECK: Name: .tst1.ren.foo{{ }}
+# CHECK: Name: .rel.tst1.ren.foo{{ }}
+# CHECK: Name: .rela.tst1.ren.bar{{ }}
+# CHECK: Name: .tst1.ren.bar{{ }}
+  - Name:  .tst1.foo
+    Type:  SHT_PROGBITS
+  - Name:  .rel.tst1.foo
+    Type:  SHT_REL
+    Info:  .tst1.foo
+  - Name:  .rela.tst1.bar
+    Type:  SHT_RELA
+    Info:  .tst1.bar
+  - Name:  .tst1.bar
+    Type:  SHT_PROGBITS
+
+## Test 2. A relocation section should be renamed together with its target
+## section even if its name does not follow the typical pattern.
+# CHECK: Name: .tst2.ren.foo{{ }}
+# CHECK: Name: .rel.tst2.ren.foo{{ }}
+  - Name:  .tst2.foo
+    Type:  SHT_PROGBITS
+  - Name:  .tst2.foo.rel
+    Type:  SHT_REL
+    Info:  .tst2.foo
+
+## Test 3. A relocation section should not be renamed if an unrelated section
+## with the same common name is renamed.
+## Note that '.rel.tst3.foo' targets '.tst3.bar', not '.tst3.foo'
+# CHECK: Name: .tst3.ren.foo{{ }}
+# CHECK: Name: .tst3.bar{{ }}
+# CHECK: Name: .rel.tst3.foo{{ }}
+  - Name:  .tst3.foo
+    Type:  SHT_PROGBITS
+  - Name:  .tst3.bar
+    Type:  SHT_PROGBITS
+  - Name:  .rel.tst3.foo
+    Type:  SHT_REL
+    Info:  .tst3.bar
+
+## Test 4. A relocation section can be renamed with an explicit --rename-section
+## command. Explicit renaming overrides implicit one.
+# CHECK: Name: .tst4.ren.foo{{ }}
+# CHECK: Name: .tst4.ren.foo.rel{{ }}
+  - Name:  .tst4.foo
+    Type:  SHT_PROGBITS
+  - Name:  .rel.tst4.foo
+    Type:  SHT_REL
+    Info:  .tst4.foo
+
+## Test 5. Should not rename a relocation section if it or its target are not
+## specified in --rename-section.
+## Note that this diverges from GNU objcopy.
+# CHECK: Name: .tst5.foo{{ }}
+# CHECK: Name: .tst5.foo.rel{{ }}
+  - Name:  .tst5.foo
+    Type:  SHT_PROGBITS
+  - Name:  .tst5.foo.rel
+    Type:  SHT_REL
+    Info:  .tst5.foo
+
+## Test 6. A dynamic relocation section should not be renamed together with its
+## target section.
+# CHECK: Name: .tst6.rela.plt{{ }}
+# CHECK: Name: .tst6.ren.got.plt{{ }}
+  - Name:  .tst6.rela.plt
+    Type:  SHT_RELA
+    Flags: [ SHF_ALLOC ]
+    Info:  .tst6.got.plt
+  - Name:  .tst6.got.plt
+    Type:  SHT_PROGBITS
+
+## Test 7. A dynamic relocation section can be renamed with an explicit
+## --rename-section command.
+# CHECK: Name: .tst7.ren.rela.plt{{ }}
+# CHECK: Name: .tst7.got.plt{{ }}
+  - Name:  .tst7.rela.plt
+    Type:  SHT_RELA
+    Flags: [ SHF_ALLOC ]
+    Info:  .tst7.got.plt
+  - Name:  .tst7.got.plt
+    Type:  SHT_PROGBITS

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
   // named in relocations.
   if (Error E = replaceAndRemoveSections(Config, ELFConfig, Obj))
     return E;
 
   if (Error E = updateAndRemoveSymbols(Config, ELFConfig, Obj))
     return E;
 
   if (!Config.SectionsToRename.empty()) {
+    std::vector<RelocationSectionBase *> RelocSections;
+    DenseSet<SectionBase *> RenamedSections;
     for (SectionBase &Sec : Obj.sections()) {
+      auto *RelocSec = dyn_cast<RelocationSectionBase>(&Sec);
       const auto Iter = Config.SectionsToRename.find(Sec.Name);
       if (Iter != Config.SectionsToRename.end()) {
         const SectionRename &SR = Iter->second;
         Sec.Name = std::string(SR.NewName);
         if (SR.NewFlags.hasValue())
           setSectionFlagsAndType(Sec, SR.NewFlags.getValue());
-      }
+        RenamedSections.insert(&Sec);
+      } else if (RelocSec && !(Sec.Flags & SHF_ALLOC))
+        // Postpone processing relocation sections which are not specified in
+        // their explicit '--rename-section' commands until 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.
+        RelocSections.push_back(RelocSec);
+    }
+
+    // Rename relocation sections according to their target sections.
+    for (RelocationSectionBase *RelocSec : RelocSections) {
+      auto Iter = RenamedSections.find(RelocSec->getSection());
+      if (Iter != RenamedSections.end())
+        RelocSec->Name = (RelocSec->getNamePrefix() + (*Iter)->Name).str();
     }
   }
 
   // Add a prefix to allocated sections and their relocation sections. This
   // should be done after renaming the section by Config.SectionToRename to
   // imitate the GNU objcopy behavior.
   if (!Config.AllocSectionsPrefix.empty()) {
     DenseSet<SectionBase *> PrefixedSections;
     for (SectionBase &Sec : Obj.sections()) {
       if (Sec.Flags & SHF_ALLOC) {