[llvm-objcopy] Emit error if removing symbol table referenced by SHT_GROUP section

[jubnzv]

Summary

SHT_GROUP sections contain a reference to a symbol indicating their "signature" symbol. The symbol table containing this symbol is referred to by the group section's sh_link field. If llvm-objcopy is instructed to remove the symbol table, it will emit an error.

This fixes https://bugs.llvm.org/show_bug.cgi?id=46153.

[jhenderson]

Thanks for taking a look @jubnzv. It's a good start, but there are some things more to look at. In particular, the test is currently broken.

llvm/test/tools/llvm-objcopy/ELF/group.test

58–59

You weren't to know this, but in our newer tests, at least in the LLVM binutils area, we tend to use '##' for comments to make them stand out better from # RUN and similar lines to do with FileCheck. Could you update here, please?

60

There's no need for >/dev/null. That throws away the stdout, which isn't a thing you need to worry about, as llvm-objcopy does not write to stdout.

Note also that the pre-merge bot indicates that your test is currently failing. Please make sure to run the test locally before updating your patch to make sure the test actually works. In this case, the problem is likely %t.o does not exist (the input file should probably be %t). If the test was passing for you locally, check your test output folder to see if there is a %t.o file (actually something like .../group.test.tmp.o) there, and delete it if there is. The test output directory can be found inside your build directory.

61

I find it a little easier to read if there is a blank line between the RUN and the CHECK (or ERR in this case) line.

62–63

llvm-objcopy doesn't modify its input file on error, so there's no need to check it here afterwards.

65

Same comment as above for '##'. Also, missing trailing '.' at the end of the sentence.

67–70

You could fold these two llvm-readobj lines together. If you switch to using llvm-readelf (or llvm-readobj --elf-output-style=GNU, which is essentially the same thing), you'll notice that it has both the sections listed, and the number of sections. I'd then check the full list rather than rely on --implicit-check-not, as it's a little safer against changing output, but it doesn't make much difference. Approximate example:

# SECTIONS:      Section header table contains 6 entries:
## FYI, no need to check the full output, just the name column. FileCheck by default doesn't need to match an entire line.
# SECTIONS-NEXT: Name
# SECTIONS-NEXT: .foo
# SECTIONS-NEXT: .bar
<etc etc>

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

971

GroupSection already has a SymTab pointer, which should point at the symbol table, if I'm not mistaken. There's no need to use the LinkSection member of Section.

As an aside, the preferred style for error messages is lower-case for the first letter too, not upper-case as you have here.

975

Similar to above, use SymTab, not LinkSection in this method.

What happens if LinkSection (later SymTab) is a nullptr? Please double check to see whether ToRemove handles nullptr or not. If it doesn't, you'll need to handle it here and will want a test that shows the behaviour in this situation.

1361–1367

I don't think you need to make these changes.

llvm/tools/llvm-objcopy/ELF/Object.h

790–792 ↗

(On Diff #272296)

It seems to me like moving this and changing the GroupSection base class is unnecessary. What's the motivation behind it?

The reason I observed in the bug that GroupSection is not a Section derivative is because Section already contains the behaviour for preventing the LinkSection being removed by default. I think better would be to not change the base class, and just implement the same error message as already exists in Section::removeSectionReferences, since we want to do something slightly differently.

[jubnzv]

@jhenderson thank you very much for your thoughtful review!

Yes, the problem with the failed test was caused by the fact that I use locally cached outputs with previous version of the test. Now I've fixed it and everything works.
I also corrected my patch according your guidances.

[jubnzv]

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

975

ToRemove is represented as find method of std::unordered_set, so it will returns past-the-end iterator if it can't find the requested key. So ToRemove could be used without additional null check.

I've also added additional test for possible regressions.

[jhenderson]

Tip: before updating the diff, select the little "Done" check box on inline comments. This will make it clear to the reviewer that you've read and addressed that comment. The "marked as done" action will then be automatically submitted when you update the diff.

llvm/test/tools/llvm-objcopy/ELF/group.test

71

Thinking about it, we should show what the value of the .group section's sh_link field here is. It's one of the later columns, so you'll want to expand this line to check up to that point (the rest of the values don't matter, so consider something like: .group {{.*}} {{.*}} ... 0. Also update the column header line to extend to the Link column.

77

I think this test case is useful in principle but needs some more work, but the description would probably be better changed to something like:

"Show that llvm-objcopy can handle a group section with a zero sh_link field".

78

I assume you didn't mean to just repeat the previous llvm-objcopy run with the exact same input, and that what you actually meant to do was show that the output of the previous run can be used as an input to llvm-objcopy again?

Assuming I've got that right, you need to update your test input/output file. I also don't think "exit with return code 0" is a particularly useful test on its own - int main(){} passes that test after all. It would be better to show that the input and output files are identical after the run.

[jubnzv]

@jhenderson thanks for the tip. I added some checks for sh_link field and updated the test.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/group.test

68–69

I'd add extra spaces to make this line up with the rows below. Additionally, I'd add the headers for the columns you have to match below, to clarify which you are actually checking.

71

I made a slight mistake in my original proposal - these should be {{.+}}. Additionally, you don't need to check anything after the fields you care about.

77

On further look, this should be "zero sh_link and sh_info fields" instead of just "a zero sh_link field" I think?

78

To assist with debugging, I'd use a different output file, so that the input file isn't overwritten. I'd them cmp the output with the input, since they should match. As things stand, this is still no better than an empty main, since you are only checking that llvm-objcopy returns 0, and not that it actually does what you expect too.

[jubnzv]

I agree, this makes it cleaner.

I also found a strange behavior of {{.*}} regex in group.test. It seems that it doesn't match an empty sequence.

For example, when I trying to match the second line here (empty Flg field):

[Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
[ 1] .group            GROUP           0000000000000000 000040 000008 04     0  0   4

This regex works as expected:

# SECTIONS-NEXT: .group    {{.+}} {{.+}}  {{.+}} {{.+}} {{.+}}     0  0

But this one doesn't match the line:

# SECTIONS-NEXT: .group    {{.+}} {{.+}}  {{.+}} {{.+}} {{.+}} {{.*}} 0  0

[jhenderson]

In D82274#2113598, @jubnzv wrote:

I agree, this makes it cleaner.

I also found a strange behavior of {{.*}} regex in group.test. It seems that it doesn't match an empty sequence.

For example, when I trying to match the second line here (empty Flg field):

[Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
[ 1] .group            GROUP           0000000000000000 000040 000008 04     0  0   4

This regex works as expected:

# SECTIONS-NEXT: .group    {{.+}} {{.+}}  {{.+}} {{.+}} {{.+}}     0  0

But this one doesn't match the line:

# SECTIONS-NEXT: .group    {{.+}} {{.+}}  {{.+}} {{.+}} {{.+}} {{.*}} 0  0

Without investigating it in too much depth, I suspect you've tripped over FileCheck's canonicalization of whitespace. Essentially, FileCheck normalises all consecutive whitespace in both the pattern and the input to match against into a single space character. Therefore, if your input is something like xyz abc, and your pattern is xyz abc FileCheck will treat them both as xyz abc, meaning it will match. I suspect, but haven't checked, that this is applied before interpreting regexes. Therefore xyz {{.*}} abc will be treated as xyz a regex matching an empty string and abc in sequence. Thus the final pattern string being used to match with will contain 2 spaces (one either side of the "empty" regex). That means that it won't match xyz abc, or xyz abc or even xyz abc (because those are all treated as having only one space). You can use --strict-whitespace to disable this canonicalization. Take a look at the documentation for details. I hope that makes sense.

Anyway, the change as-is LGTM.

[jubnzv]

@jhenderson thanks for helping me. I'll remember these tips for next time.

Could you please commit this for me?

[Higuoxing]

LGTM with one nit.

llvm/test/tools/llvm-objcopy/ELF/group.test

59

'have the a group' -> 'have a'

[jhenderson]

In D82274#2118335, @jubnzv wrote:

@jhenderson thanks for helping me. I'll remember these tips for next time.

Could you please commit this for me?

Can do. What do you want for your Git author name and email?

[jubnzv]

In D82274#2119252, @jhenderson wrote:

In D82274#2118335, @jubnzv wrote:

@jhenderson thanks for helping me. I'll remember these tips for next time.

Could you please commit this for me?

Can do. What do you want for your Git author name and email?

Georgy Komarov <[e-mail]>

llvm/test/tools/llvm-objcopy/ELF/group.test

 # RUN: yaml2obj %s -o %t
-# RUN: llvm-objcopy --remove-section=.text.bar %t %t2
-# RUN: llvm-readobj --elf-section-groups %t2 | FileCheck %s
+# RUN: llvm-objcopy --remove-section=.text.bar %t %t1
+# RUN: llvm-readobj --elf-section-groups %t1 | FileCheck %s
 
 # In this test the section .text.bar is getting removed, as a result, 
 # the indices of the sections which go after .text.bar will change, 
 # thus the fields Link, Info and the content of .group should be updated.
 
 # CHECK:           Name: .group
 # CHECK-NEXT:      Index: 1
 # CHECK-NEXT:      Link: 3
- Name:            bar
     Section:         .text.bar
     Size:            0x0000000000000000
     Binding:         STB_WEAK
   - Name:            foo
     Type:            STT_FUNC
     Section:         .text.foo
     Size:            0x0000000000000000
     Binding:         STB_WEAK
+
+## This checks that tool will emit an error when trying to remove the symbol
+## table when we have the a group section linked with symtab.
+# RUN: not llvm-objcopy -R .symtab %t %t2 2>&1 | FileCheck %s --check-prefix=ERR -DINPUT=%t
+
+# ERR: error: '[[INPUT]]': section '.symtab' cannot be removed because it is referenced by the group section '.group'
+
+## The '.symtab' section could be removed using --allow-broken-links option.
+# RUN: llvm-objcopy --allow-broken-links -R .symtab %t %t3
+# RUN: llvm-readelf --sections %t3 | FileCheck %s --check-prefix=SECTIONS
+
+# SECTIONS: There are 6 section headers
+# SECTIONS: Name
+# SECTIONS-NEXT: NULL
+# SECTIONS-NEXT: .group
+# SECTIONS-NEXT: .text.bar
+# SECTIONS-NEXT: .text.foo
+# SECTIONS-NEXT: .strtab
+# SECTIONS-NEXT: .shstrtab
+
+## Nothing will change when removing symtab again; tool should exit with return code 0.
+# RUN: llvm-objcopy --allow-broken-links -R .symtab %t %t3

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

if (ToRemove(LinkSection)) {
     LinkSection = nullptr;
   }
   return Error::success();
 }
 
 void GroupSection::finalize() {
   this->Info = Sym->Index;
   this->Link = SymTab->Index;
 }
 
 Error GroupSection::removeSectionReferences(
     bool AllowBrokenLinks, function_ref<bool(const SectionBase *)> ToRemove) {
+  if (ToRemove(SymTab) && !AllowBrokenLinks) {
+    return createStringError(
+        llvm::errc::invalid_argument,
+        "section '.symtab' cannot be removed because it is "
+        "referenced by the group section '%s'",
+        this->Name.data());
+  }
   llvm::erase_if(GroupMembers, ToRemove);
   return Error::success();
 }
 
 Error GroupSection::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
   if (ToRemove(*Sym))
     return createStringError(llvm::errc::invalid_argument,
                              "symbol '%s' cannot be removed because it is "
auto SymTab = SecTable.template getSectionOfType<SymbolTableSection>(
           GroupSec->Name + "' is not a symbol table");
   Symbol *Sym = SymTab->getSymbolByIndex(GroupSec->Info);
   if (!Sym)
     error("info field value '" + Twine(GroupSec->Info) + "' in section '" +
           GroupSec->Name + "' is not a valid symbol index");
   GroupSec->setSymTab(SymTab);
   GroupSec->setSymbol(Sym);
   if (GroupSec->Contents.size() % sizeof(ELF::Elf32_Word) ||
       GroupSec->Contents.empty())
     error("the content of the section " + GroupSec->Name + " is malformed");
   const ELF::Elf32_Word *Word =
       reinterpret_cast<const ELF::Elf32_Word *>(GroupSec->Contents.data());
   const ELF::Elf32_Word *End =
       Word + GroupSec->Contents.size() / sizeof(ELF::Elf32_Word);
   GroupSec->setFlagWord(*Word++);
   for (; Word != End; ++Word) {
     uint32_t Index = support::endian::read32<ELFT::TargetEndianness>(Word);
     GroupSec->addMember(SecTable.getSection(
         Index, "group member index " + Twine(Index) + " in section '" +
                    GroupSec->Name + "' is invalid"));
   }
 }