[PATCH] D112116: [llvm-objcopy] Add --update-section

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:56
+    Content:         "3030303030303030"
+  - Name:            .bss
+    Type:            SHT_NOBITS
----------------
jhenderson wrote:
> Perhaps call this ".unsupported_type", for full detail.
> 
> Since it's not in a segment, it doesn't need an address.
Actually, now that you've got a SHT_NULL type, I'd call it `.nobits_type`. I believe it also doesn't need a size?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:34
+## We can't update certain types of sections.
+# RUN: not llvm-objcopy --update-section=.unsupported_type=%t.same %t %t-err2 2>&1 | FileCheck %s --check-prefix=ERR-NOBITS-TYPE
+# RUN: not llvm-objcopy --update-section=.null_type=%t.same %t %t-err2 2>&1 | FileCheck %s --check-prefix=ERR-NULL-TYPE
----------------
Here and elsewhere, I'd recommend breaking up these run lines as they're getting quite long, as per the inline edit. You don't need to be strict about the length, but the 80 character limit for code is a good rough guideline.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:42
+## But we can insert larger data if the section is NOT part of a segment.
+# RUN: echo -n 111122223 > %t.larger
+# RUN: llvm-objcopy --update-section=.not_in_segment=%t.larger %t %t.larger.o
----------------
Don't need to re-echo this line, since you already have `%t.larger`. Maybe move all the echoes up to the start of the file, so you only have the llvm-objcopy run lines by this point.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:51
+
+# RUN: not llvm-objcopy --update-section %t %t.out 2>&1 | FileCheck %s --check-prefix=MISSING-EQ
+# RUN: not llvm-objcopy --update-section=.in_segment= %t %t.out 2>&1 | FileCheck %s --check-prefix=MISSING-FILE
----------------
Perhaps add a brief comment before these two cases saying something like "test option parsing failures" or something to that effect.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:14
+
+# Update segment section with a smaller chunk of data. This will only overwrite the start of the original data.
+# RUN: echo -n 11 > %t3.text
----------------
jhenderson wrote:
> I think this does mean the section size as recorded in the section header is reduced though, right? Probably say "This will only overwrite the start of the original data and adjust the section header."
> 
> Should we also have a test case without a segment?
It's a bit fiddly, but I think you should test the bytes that appear immediately after this section to show that the contents are unchanged. It's not necessarily an issue if they are/aren't changed, but I think we need to make a conscious decision should this behaviour ever be changed, hence the test.

Also, some more testing that occurred to me: we should have a section immediately following the to-be-shrunk section, and show that its offset hasn't changed (or more specifically, its offset relative to the segment start), to show that the shrinking doesn't cause the other section to bunch up.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2149
+        errc::invalid_argument,
+        "section '%s' can't be updated because it does not occupy file space",
+        Name.str().c_str());
----------------
Maybe "does not have contents"? That seems to be more the specific issue here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112116