This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][ELF] Fix removing a group member section.
ClosedPublic

Authored by ikudrin on May 26 2020, 8:32 AM.

Details

Summary

When a group member is removed, the corresponding record in the SHT_GROUP section has to be also deleted.

This fixes PR46064.

Diff Detail

Event Timeline

ikudrin created this revision.May 26 2020, 8:32 AM
MaskRay accepted this revision.May 26 2020, 11:17 PM

LG, but please wait for other opinions.

llvm/test/tools/llvm-objcopy/ELF/remove-section-in-group.test
10

The excessive indentation should be deleted.

This revision is now accepted and ready to land.May 26 2020, 11:17 PM
jhenderson added inline comments.May 27 2020, 1:47 AM
llvm/test/tools/llvm-objcopy/ELF/remove-section-in-group.test
6

I'd probably move this comment above the RUN bit, since it describes the test as a whole.

17

I suspect (don't know for sure) that you can omit the Link line here. I think yaml2obj adds it automatically by default for these sort of sections.

33

I personally find it easier to follow a test if the CHECK is closer to where it is used by FileCheck. I'd layout the file (with optional new lines at appropriate spots):

## Comments
# RUN: ...
# CHECK:

<yaml>
llvm/tools/llvm-objcopy/ELF/Object.cpp
973

What happens if you remove the symbol table itself? I'm slightly concerned that it could leave the sh_link field invalid as presumably Section::removeSectionReferences won't be called for the GroupSection.

ikudrin marked 5 inline comments as done.May 29 2020, 6:26 AM
ikudrin added inline comments.
llvm/tools/llvm-objcopy/ELF/Object.cpp
973

That is another issue with group sections, but not something which is introduced by this patch. Note that the base class for GroupSection is SectionBase, not Section. We need another patch for that.

This revision was automatically updated to reflect the committed changes.
jhenderson added inline comments.May 29 2020, 6:40 AM
llvm/tools/llvm-objcopy/ELF/Object.cpp
973

Thanks. I suspected something like that might have been the case. I guess it's something that needs fixing. Are you going to fix it shortly, or shall I file a bug?

ikudrin marked an inline comment as done.May 29 2020, 7:22 AM
ikudrin added inline comments.
llvm/tools/llvm-objcopy/ELF/Object.cpp
973

Honestly, fixing that is a bit off my plans.