[llvm-objcopy] Remove empty SHT_GROUP sections #97141

chestnykh · 2024-06-29T06:33:29Z

Currently llvm-objcopy/llvm-strip in --strip-debug mode doesn't remove such sections. This behavior can lead to incompatibilities with GNU binutils (for examples ld.bfd before https://sourceware.org/PR20520 cannot process the object file contains empty .group section).
The ELF object that contains group section with .debug_* sections inside can be obtained by gcc -g3.
Fix #97139

github-actions · 2024-06-29T06:33:45Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-06-29T06:34:14Z

@llvm/pr-subscribers-llvm-binary-utilities

Author: Dmitriy Chestnykh (chestnykh)

Changes

Currently llvm-strip in --strip-debug mode doesn't remove such sections. This behavior can lead to incompatibilities with GNU binutils (for examples ld.bfd cannot process the object file contains empty .group section).
The ELF object that contains group section with .debug_* sections inside can be obtained by gcc -g3.
Fix #97139

Full diff: https://github.com/llvm/llvm-project/pull/97141.diff

3 Files Affected:

(modified) llvm/lib/ObjCopy/ELF/ELFObject.cpp (+9-1)
(modified) llvm/lib/ObjCopy/ELF/ELFObject.h (+2)
(added) llvm/test/tools/llvm-objcopy/ELF/strip-debug-empty-group.test (+25)

diff --git a/llvm/lib/ObjCopy/ELF/ELFObject.cpp b/llvm/lib/ObjCopy/ELF/ELFObject.cpp
index 02591e6f987c2..44181ccd328bf 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObject.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObject.cpp
@@ -2206,8 +2206,16 @@ Error Object::removeSections(
   // Transfer removed sections into the Object RemovedSections container for use
   // later.
   std::move(Iter, Sections.end(), std::back_inserter(RemovedSections));
-  // Now finally get rid of them all together.
+  // Now get rid of them all together.
   Sections.erase(Iter, std::end(Sections));
+
+  // Finally iterate over all sections and erase empty SHT_GROUP sections.
+  for (auto Iter = Sections.begin(); Iter != Sections.end(); ++Iter) {
+    if (auto GroupSec = dyn_cast<GroupSection>(Iter->get())) {
+      if (GroupSec->getMembersCount() == 0)
+        Sections.erase(Iter);
+    }
+  }
   return Error::success();
 }
 
diff --git a/llvm/lib/ObjCopy/ELF/ELFObject.h b/llvm/lib/ObjCopy/ELF/ELFObject.h
index 2b1895a30b41e..9bbef268017ea 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObject.h
+++ b/llvm/lib/ObjCopy/ELF/ELFObject.h
@@ -963,6 +963,8 @@ class GroupSection : public SectionBase {
       const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
   void onRemove() override;
 
+  size_t getMembersCount() {return GroupMembers.size(); }
+
   static bool classof(const SectionBase *S) {
     return S->OriginalType == ELF::SHT_GROUP;
   }
diff --git a/llvm/test/tools/llvm-objcopy/ELF/strip-debug-empty-group.test b/llvm/test/tools/llvm-objcopy/ELF/strip-debug-empty-group.test
new file mode 100644
index 0000000000000..26bbde0c603bb
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/strip-debug-empty-group.test
@@ -0,0 +1,25 @@
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-strip --strip-debug %t -o %t1
+# RUN: llvm-readelf --section-groups %t1 | FileCheck %s
+
+--- !ELF
+FileHeader:
+  Class:      ELFCLASS64
+  Data:       ELFDATA2LSB
+  Type:       ET_REL
+  Machine:    EM_X86_64
+Sections:
+  - Name:     .group
+    Type:     SHT_GROUP
+    Info:     foo_grp
+    Members:
+      - SectionOrType:  GRP_COMDAT
+      - SectionOrType:  .debug_macro
+  - Name:     .debug_macro
+    Type:     SHT_PROGBITS
+    Flags:    [ SHF_GROUP ]
+Symbols:
+  - Name:     foo_grp
+    Section:  .group
+
+# CHECK: There are no section groups in this file.

chestnykh · 2024-06-29T07:36:07Z

CC @MaskRay

MaskRay · 2024-06-29T16:43:58Z

Currently llvm-strip in --strip-debug mode doesn't remove such sections.

Thanks for noticing this. This applies to other section removal operations like --remove-section, so another test will be useful

chestnykh · 2024-06-29T19:16:58Z

Currently llvm-strip in --strip-debug mode doesn't remove such sections.

Thanks for noticing this. This applies to other section removal operations like --remove-section, so another test will be useful

Added some new runs of llvm-strip + llvm-readelf

MaskRay

You can reopen this PR...

MaskRay · 2024-06-29T16:41:11Z

llvm/test/tools/llvm-objcopy/ELF/strip-debug-empty-group.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-strip --strip-debug %t -o %t1
+# RUN: llvm-readelf --section-groups %t1 | FileCheck %s
+


Consider adding another test using --remove-section=.debug_macro

The test file can probably be merged into remove-section-in-group.test

Moved test to remove-section-in-group.test. Added --remove-section=.debug_macro test (line 44).

github-actions · 2024-06-30T18:33:04Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jh7370

I think it would be worth a test case where you explicitly remove an empty .group section too, to show that the code handles that successfully (i.e. it doesn't try to remove the same .group section twice and blow up).

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

llvm/lib/ObjCopy/ELF/ELFObject.cpp

jh7370

LGTM.

MaskRay · 2024-07-06T02:56:58Z

llvm/lib/ObjCopy/ELF/ELFObject.cpp

+  for (auto Iter = Sections.begin(); Iter != Sections.end(); ++Iter)
+    if (auto GroupSec = dyn_cast<GroupSection>(Iter->get()))
+      if (GroupSec->getMembersCount() == 0)
+        Sections.erase(Iter);


This is UB and has a quadratic time complexity if the erased elements are many. https://eel.is/c++draft/vector#modifiers-3

"Effects: Invalidates iterators and references at or after the point of the erase."

You could utilize the return address of erase, but to address the time complexity issue you'd need erase_if anyway.

Fixed, thanks!

MaskRay · 2024-07-06T04:54:32Z

llvm/lib/ObjCopy/ELF/ELFObject.h

@@ -963,6 +963,8 @@ class GroupSection : public SectionBase {
      const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
  void onRemove() override;

+  size_t getMembersCount() { return GroupMembers.size(); }


Currently llvm-strip in --strip-debug mode doesn't remove such sections. This behavior can lead to incompatibilities with GNU binutils (for examples ld.bfd cannot process the object file contains empty .group section). The ELF object that contains group section with .debug_* sections inside can be obtained by gcc -g3. Fix llvm#97139

jh7370

LGTM again.

chestnykh · 2024-07-08T17:13:13Z

@MaskRay can you merge this PR?

github-actions · 2024-07-08T18:50:44Z

@chestnykh Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

This reverts commit 359c64f. This caused heap-use-after-free. See #98106.

This reverts commit 359c64f. This caused heap-use-after-free. See llvm#98106.

llvmbot added the llvm:binary-utilities label Jun 29, 2024

chestnykh force-pushed the strip-empty-group branch 2 times, most recently from 0e46333 to ce96728 Compare June 29, 2024 06:37

chestnykh changed the title ~~Remove empty SHT_GROUP sections.~~ [llvm-strip] Remove empty SHT_GROUP sections. Jun 29, 2024

tschuett requested a review from MaskRay June 29, 2024 12:10

MaskRay requested a review from jh7370 June 29, 2024 16:44

chestnykh force-pushed the strip-empty-group branch from ce96728 to 8bc990f Compare June 29, 2024 19:15

chestnykh force-pushed the strip-empty-group branch from 8bc990f to a6e39db Compare June 29, 2024 20:16

chestnykh closed this Jun 30, 2024

chestnykh force-pushed the strip-empty-group branch from a6e39db to eed7c5e Compare June 30, 2024 09:52

chestnykh mentioned this pull request Jun 30, 2024

[llvm-strip] Remove empty SHT_GROUP sections. #97204

Closed

MaskRay reopened this Jun 30, 2024

MaskRay reviewed Jun 30, 2024

View reviewed changes

chestnykh force-pushed the strip-empty-group branch 3 times, most recently from af93768 to 648809a Compare June 30, 2024 18:47

jh7370 reviewed Jul 4, 2024

View reviewed changes

llvm/test/tools/llvm-objcopy/ELF/remove-section-in-group.test Outdated Show resolved Hide resolved

llvm/lib/ObjCopy/ELF/ELFObject.cpp Outdated Show resolved Hide resolved

chestnykh force-pushed the strip-empty-group branch from 648809a to f0108b1 Compare July 4, 2024 09:41

jh7370 approved these changes Jul 4, 2024

View reviewed changes

chestnykh requested a review from MaskRay July 5, 2024 02:36

MaskRay requested changes Jul 6, 2024

View reviewed changes

chestnykh force-pushed the strip-empty-group branch from f0108b1 to 49172a9 Compare July 6, 2024 04:51

chestnykh requested review from MaskRay and jh7370 July 6, 2024 04:52

MaskRay reviewed Jul 6, 2024

View reviewed changes

MaskRay approved these changes Jul 6, 2024

View reviewed changes

chestnykh force-pushed the strip-empty-group branch from 49172a9 to fbd81eb Compare July 6, 2024 05:03

jh7370 approved these changes Jul 8, 2024

View reviewed changes

MaskRay changed the title ~~[llvm-strip] Remove empty SHT_GROUP sections.~~ [llvm-objcopy] Remove empty SHT_GROUP sections Jul 8, 2024

MaskRay merged commit 359c64f into llvm:main Jul 8, 2024
5 of 7 checks passed

pranavk mentioned this pull request Jul 9, 2024

[llvm-objcopy] Remove references for empty section groups #98106

Merged

MaskRay added a commit that referenced this pull request Jul 9, 2024

Revert "[llvm-objcopy] Remove empty SHT_GROUP sections (#97141)"

2abe53a

This reverts commit 359c64f. This caused heap-use-after-free. See #98106.

aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024

Revert "[llvm-objcopy] Remove empty SHT_GROUP sections (llvm#97141)"

d9ce1d2

This reverts commit 359c64f. This caused heap-use-after-free. See llvm#98106.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[llvm-objcopy] Remove empty SHT_GROUP sections #97141

[llvm-objcopy] Remove empty SHT_GROUP sections #97141

chestnykh commented Jun 29, 2024 •

edited by MaskRay

Loading

github-actions bot commented Jun 29, 2024

llvmbot commented Jun 29, 2024

chestnykh commented Jun 29, 2024

MaskRay commented Jun 29, 2024

chestnykh commented Jun 29, 2024

MaskRay left a comment

MaskRay Jun 29, 2024

chestnykh Jun 30, 2024

github-actions bot commented Jun 30, 2024 •

edited

Loading

jh7370 left a comment

jh7370 left a comment

MaskRay Jul 6, 2024

chestnykh Jul 6, 2024

MaskRay Jul 6, 2024

chestnykh Jul 6, 2024

jh7370 left a comment

chestnykh commented Jul 8, 2024

github-actions bot commented Jul 8, 2024

[llvm-objcopy] Remove empty SHT_GROUP sections #97141

[llvm-objcopy] Remove empty SHT_GROUP sections #97141

Conversation

chestnykh commented Jun 29, 2024 • edited by MaskRay Loading

github-actions bot commented Jun 29, 2024

llvmbot commented Jun 29, 2024

chestnykh commented Jun 29, 2024

MaskRay commented Jun 29, 2024

chestnykh commented Jun 29, 2024

MaskRay left a comment

Choose a reason for hiding this comment

MaskRay Jun 29, 2024

Choose a reason for hiding this comment

chestnykh Jun 30, 2024

Choose a reason for hiding this comment

github-actions bot commented Jun 30, 2024 • edited Loading

jh7370 left a comment

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

MaskRay Jul 6, 2024

Choose a reason for hiding this comment

chestnykh Jul 6, 2024

Choose a reason for hiding this comment

MaskRay Jul 6, 2024

Choose a reason for hiding this comment

chestnykh Jul 6, 2024

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

chestnykh commented Jul 8, 2024

github-actions bot commented Jul 8, 2024

chestnykh commented Jun 29, 2024 •

edited by MaskRay

Loading

github-actions bot commented Jun 30, 2024 •

edited

Loading