[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-error.test:1
+## Test various errors around --update-section argument parsing.
+
----------------
Up to you, but these days, we tend to add error case checks for argument parsing in the same test as testing the normal behaviour of the option.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:4
+## Update the segment section with a regular chunk of data the same size as the section.
+# RUN: echo -n 11112222 > %t.text
+# RUN: llvm-objcopy --update-section=.text=%t.text %t %t2
----------------
It might be useful to name these files "%t.smaller", "%t.same" and "%t.bigger" or something similar, so that it's obvious from the specific run line what the test is testing.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:8
+
+## This should test that if we overwrite a given section (.text) with new data, then rewrite it
+## back (from the second `--update-section`), then it should be the exact same as the original file.
----------------
"Show that if we overwrite...".

Do both operations actually happen, or is it just "last one wins"? In particular, I'm thinking of the case where you attempt to grow a section in a segment (which should fail) and then update it again back to its original (or smaller) size, in the same invocation. In this case, you could make an argument that it should just work, because you can ignore all but the last `--update-section` for any given named section. (I can also see an argument that both are applied though, so you'd get an error). Thoughts?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:52
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
----------------
Perhaps call this ".in_segment", for full detail.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:54
+    Type:            SHT_PROGBITS
+    Address:         0x1000
+    Content:         "3030303030303030"
----------------
You may be able to get away with the default `Address` for this section and in the program header. Double-check the generated object to make sure the section does appear in the segment, but I think it should just work.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:56
+    Content:         "3030303030303030"
+  - Name:            .bss
+    Type:            SHT_NOBITS
----------------
Perhaps call this ".unsupported_type", for full detail.

Since it's not in a segment, it doesn't need an address.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:60
+    Address:         0x1018
+  - Name:            .text2
+    Type:            SHT_PROGBITS
----------------
Perhaps call this ".not_in_segment", for full detail.

Since it's not in a segment, it doesn't need an address.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:71
+# NORMAL:   Address: 0x1000
+# NORMAL:   Size: 8
+# NORMAL:   |11112222|
----------------
I may be being stupid, but the new size looks like it's 4 bytes? Are the section data bytes characters or hex numbers?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:117
+# ERR1: error: {{.*}}section '.nosection' not found
+# ERR2: error: {{.*}}section '.bss' can't be updated
+# ERR3: error: {{.*}}cannot fit data of size 9 into section '.text' with size 8 that is part of a segment
----------------
Probably need to state why it can't be updated here.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:553
+handleUserSection(StringRef Flag,
+                  llvm::function_ref<Error(StringRef, ArrayRef<uint8_t>)> F) {
+  std::pair<StringRef, StringRef> SecPair = Flag.split("=");
----------------
Do you need the explicit `llvm::`?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2112
+    SectionBase *Sec = it.first;
+    const std::vector<uint8_t> &Data = it.second;
+
----------------
Use `ArrayRef<uint8_t>` here.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2146
+  auto *OldSec = It->get();
+
+  if (!OldSec->hasContents())
----------------
I'd delete this blank line.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:498
+  bool hasContents() const override {
+    return Type != ELF::SHT_NOBITS && Type != ELF::SHT_NULL;
+  }
----------------
Need a test case for SHT_NULL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112116