This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][ELF] Fix removing ".group" sections.
ClosedPublic

Authored by ikudrin on May 25 2020, 2:36 AM.

Details

Summary

When a SHT_GROUP section is removed, but other sections of the group are kept, the SHF_GROUP flag of these sections should be dropped, otherwise the resulting ELF file will be malformed.

Diff Detail

Event Timeline

ikudrin created this revision.May 25 2020, 2:36 AM
ikudrin retitled this revision from [llvm-objcopy] Fix removing ".group" sections. to [llvm-objcopy][ELF] Fix removing ".group" sections..May 25 2020, 2:57 AM

The code looks good. We have another issue: https://bugs.llvm.org/show_bug.cgi?id=46064 (if a member is deleted, SHT_GROUP content needs a change)

GNU objcopy was fixed by https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=30288845d67c64d07905c1e4ca9de4768d3b2dd8

llvm/test/tools/llvm-objcopy/ELF/remove-group-section.test
1

I'd prefer remove-section-group.test because the operation is remove-section.

(SHT_GROUP is referred to as "section group")

29

Sections [ this line can be deleted

31

Indent keys. See other files for examples

ikudrin updated this revision to Diff 266104.May 25 2020, 10:39 PM
ikudrin marked 3 inline comments as done.
  • Updated the test according to @MaskRay's comments. Thanks!

Sounds reasonable to me. I don't think this behaviour (removing .group sections) has been considered before. Some testing related to that behaviour that we should probably have, but which is outside the scope of this change is to show that you can remove a signature symbol, if the .group section has been removed (assuming that it works of course). Something else that I'm wondering perhaps DOES belong with this change - should we test that if you remove a group section in the same run as removing a member of that group no problems occur? I could imagine an issue with dangling pointers if we didn't do things properly.

llvm/test/tools/llvm-objcopy/ELF/remove-section-group.test
6–7 ↗(On Diff #266104)

Nit: use '##' for comments for easy distinction from test commands.

29 ↗(On Diff #266104)

Up to you, but you can probably also delete this line too, as you are only dumping sections.

ikudrin updated this revision to Diff 266220.May 26 2020, 8:17 AM
ikudrin marked 2 inline comments as done.

Sounds reasonable to me. I don't think this behaviour (removing .group sections) has been considered before. Some testing related to that behaviour that we should probably have, but which is outside the scope of this change is to show that you can remove a signature symbol, if the .group section has been removed (assuming that it works of course).

As far as I can see, the symbol is removed.

Something else that I'm wondering perhaps DOES belong with this change - should we test that if you remove a group section in the same run as removing a member of that group no problems occur? I could imagine an issue with dangling pointers if we didn't do things properly.

The objects which represent the sections are not actually deleted, just moved to Object::RemovedSections to be used later in ELFWriter<ELFT>::writeSegmentData(). Thus, there are no dangling pointers.

jhenderson accepted this revision.May 27 2020, 1:38 AM

Something else that I'm wondering perhaps DOES belong with this change - should we test that if you remove a group section in the same run as removing a member of that group no problems occur? I could imagine an issue with dangling pointers if we didn't do things properly.

The objects which represent the sections are not actually deleted, just moved to Object::RemovedSections to be used later in ELFWriter<ELFT>::writeSegmentData(). Thus, there are no dangling pointers.

Indeed, that's the current state of the code, so it should work, but it won't necessarily apply in the event of a future refactor. Anyway, LGTM, since this is not really part of this change.

This revision is now accepted and ready to land.May 27 2020, 1:38 AM
MaskRay added inline comments.May 27 2020, 8:06 AM
llvm/test/tools/llvm-objcopy/ELF/remove-section-group.test
29 ↗(On Diff #266104)

Move CHECK: before the YAML text as people seem to think the following order

RUN:
CHECK:
source

is easier to read.

ikudrin marked an inline comment as done.May 29 2020, 6:26 AM
This revision was automatically updated to reflect the committed changes.