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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:58-59
+
+# 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.o %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR -DINPUT=%t.o
----------------
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?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:60
+# table when we have the a group section linked with symtab.
+# RUN: not llvm-objcopy -R .symtab %t.o %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR -DINPUT=%t.o
+# ERR: error: '[[INPUT]]': section '.symtab' cannot be removed because it is referenced by the group section '.group'
----------------
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.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:61
+# RUN: not llvm-objcopy -R .symtab %t.o %t2 2>&1 >/dev/null | FileCheck %s --check-prefix=ERR -DINPUT=%t.o
+# ERR: error: '[[INPUT]]': section '.symtab' cannot be removed because it is referenced by the group section '.group'
+# RUN: llvm-readobj --file-headers --sections %t | FileCheck %s --check-prefix=CHECKEXIST
----------------
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.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:62-63
+# ERR: error: '[[INPUT]]': section '.symtab' cannot be removed because it is referenced by the group section '.group'
+# RUN: llvm-readobj --file-headers --sections %t | FileCheck %s --check-prefix=CHECKEXIST
+# CHECKEXIST: SectionHeaderCount: 7
+
----------------
llvm-objcopy doesn't modify its input file on error, so there's no need to check it here afterwards.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:65
+
+# The '.symtab' section could be removed using --allow-broken-links option
+# RUN: llvm-objcopy --allow-broken-links -R .symtab %t.o %t3
----------------
Same comment as above for '##'. Also, missing trailing '.' at the end of the sentence.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:67-70
+# RUN: llvm-readobj --sections %t3 | FileCheck %s --check-prefix=SECTIONS --implicit-check-not=.symtab
+# SECTIONS:      Name: .group
+# RUN: llvm-readobj --file-headers --sections %t3 | FileCheck %s --check-prefix=CHECKREMOVED
+# CHECKREMOVED: SectionHeaderCount: 6
----------------
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>
```


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:971
+    return;
+  LinkSection = SecTable.getSection(Link, "Link field value " + Twine(Link) +
+                                              " in group section " + Name +
----------------
`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.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:983
     bool AllowBrokenLinks, function_ref<bool(const SectionBase *)> ToRemove) {
+  if (ToRemove(LinkSection)) {
+    if (!AllowBrokenLinks)
----------------
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.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1369-1374
+  const ArrayRef<uint8_t> Contents = GroupSec->getContents();
+  if (Contents.size() % sizeof(ELF::Elf32_Word) || 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);
+      reinterpret_cast<const ELF::Elf32_Word *>(Contents.data());
+  const ELF::Elf32_Word *End = Word + Contents.size() / sizeof(ELF::Elf32_Word);
----------------
I don't think you need to make these changes.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:790-792
-  // TODO: Contents is present in several classes of the hierarchy.
-  // This needs to be refactored to avoid duplication.
-  ArrayRef<uint8_t> Contents;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82274