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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked 5 inline comments as done.
rupprecht added inline comments.


================
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
----------------
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.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1640
 void Object::sortSections() {
-  // Put all sections in offset order. Maintain the ordering as closely as
-  // possible while meeting that demand however.
+  // Use stable_sort to maintain the original ordering as closely as possible.
   llvm::stable_sort(Sections, [](const SecPtr &A, const SecPtr &B) {
----------------
jakehehrlich wrote:
> MaskRay wrote:
> > @jakehehrlich I can't find a use site of `objcopy::elf::Object::Sections` that requires it to be sorted.
> > 
> > I guess we can probably just delete this function and fix the 5 or 6 tests that check the section order.
> The layout algorithm assumes that things are sorted by offset so we can't just delete this but we can sort a copy there instead.
I don't understand this either. I need to study it in more detail, but `Object::layoutSections` iterates over sections (in the order as sorted here) and assigns offsets based on what the aligned value of (previous offset + previous size) is... that doesn't require them to be sorted, AFAICT.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1643
+    // Put SHT_GROUP sections first, since group section headers must come
+    // before the sections they contain. This also matches what GNU objcopy
+    // does.
----------------
MaskRay wrote:
> MaskRay wrote:
> > > This also matches what GNU objcopy does.
> > 
> > This is not accurate.. I suspect it just doesn't sort sections, unless in --pad-to= or --gap-fill= mode.
> Sorry, GNU objcopy does move SHT_GROUP sections before.
> 
> bfd/elf.c:assign_section_numbers does this. It has a comment:
> 
> `      /* Put SHT_GROUP sections first.  */`
Correct; sections are rearranged (for `SHT_GROUP`) but not sorted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62620