[PATCH] D40523: [llvm-objcopy] Add --only-keep-debug

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

As well as the new inline comments, there are a number of older points that need addressing still, when you next make changes.

It's not clear to me why you remove the new NOBITS sections from the segments. We don't remove other NOBITS sections from segments after all.

I did some thinking, and I'm pretty confident I understand more about the need for section offsets to change to the start of the segment (i.e. point 3 above). By not moving the section offset, but reducing the segment file size, the section to segment mapping (as displayed with readelf -l) no longer matches following stripping, whereas GNU objcopy behaviour (i.e. moving the section to the start offset of the segment) means that it continues to match (readelf appears to treat zero-sized sections at the end of a segment as being in that segment). Furthermore, if you consider the case where the sections were NOBITS in the linker input, they would be emitted at the start offsets. Not moving the sections is starting to feel simply wrong to me.



================
Comment at: test/tools/llvm-objcopy/only-keep-debug-phdrs.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy -only-keep-debug %t %t2
----------------
Could you please either extend this test, or add a new test that demonstrates the behaviour when a non-alloc section is in a segment.


================
Comment at: test/tools/llvm-objcopy/only-keep-debug-phdrs.test:3
+# RUN: llvm-objcopy -only-keep-debug %t %t2
+# RUN: llvm-readobj -file-headers -sections -symbols -program-headers %t2 | FileCheck %s
+
----------------
You might want to break this line over multiple lines using the '\' end-of-line mechanism.


================
Comment at: test/tools/llvm-objcopy/only-keep-debug-phdrs.test:15-16
+    Content:         "00000000"
+  - Name:            .blarg
+    Type:            SHT_PROGBITS
+  - Name:            .readonly
----------------
Is this section supposed to be empty explicitly? If so, could you rename it .empty (to make the intention explicit) or similar, and if not, please add some content.


================
Comment at: test/tools/llvm-objcopy/only-keep-debug-phdrs.test:59
+
+# CHECK: Name: .debugfoo
+# CHECK: Name: .blarg
----------------
You should check the address and offsets of these sections, since we are messing about with layout with this switch.


================
Comment at: test/tools/llvm-objcopy/only-keep-debug-phdrs.test:83
+# CHECK-NEXT:     Name: foo
+# CHECK-NEXT:     Value:
+# CHECK-NEXT:     Size:
----------------
You should check the value of these symbols, for the same reason as testing the section properties.


================
Comment at: test/tools/llvm-objcopy/only-keep-debug-phdrs.test:103-105
+# CHECK-NEXT: Offset
+# CHECK-NEXT: Virtual
+# CHECK-NEXT: Physical
----------------
I think these fields need testing as well for the program headers, to show the behaviour.


================
Comment at: tools/llvm-objcopy/Object.cpp:695
+  // make whole new section and delete the old way. This means removing any
+  // remaining references and replacing them with the new one as well.
+  for (auto &Sec : Sections) {
----------------
with the new -> with references to the new


Repository:
  rL LLVM

https://reviews.llvm.org/D40523