[PATCH] D62620: [llvm-objcopy] Fix SHT_GROUP ordering.

Thu Jun 6 23:07:03 PDT 2019

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

I don't have more comments. This does make llvm-objcopy behave more like GNU objcopy and fixes the SHT_GROUP bug. Such object files are accepted by ld.bfd and lld as a natural extension, but gold takes another approach to process SHT_GROUP and would give an error.

I slightly prefer deleting `Object::sortSections()` (I can't find anywhere the sorted behavior is required by the layout algorithm) but the current change is also good.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group-reorder.test:1
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-objcopy %t.o %t.2.o
----------------
rupprecht wrote:
> MaskRay wrote:
> > Regarding this test, I'm not sure if llvm-objcopy should repair the object file. It seems currently the only broken tool is us :( After this issue is fixed, the repair functionality may be use useful any more...
> Mind clarify what's broken in the object file? The input here is valid -- .group contains .bar, and comes before it -- but the test shows that .group moves to index 1.
> 
> It may also repair a broken object file, but I didn't test if it does that. But this should fix a bug by not *creating* a broken object file.
Sorry you can ignore my previous comment. I made the comment before realizing sorting is the behavior of GNU objcopy.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62620