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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section-error.test:1
+## Test various errors around --update-section argument parsing.
+
----------------
jhenderson wrote:
> 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.
Fair. Moved to the original test file.


================
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.
----------------
jhenderson wrote:
> "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?
I see. Personally I'd prefer getting the error on the size growing and not allowing last-one-wins. I think having a situation where someone does `--update-section=.text=larger.txt --update-section=.text=smaller.txt`passing, then deciding to remove `--update-section=.text=smaller.txt` (which leads to the error) be kind of confusing. I'd like to avoid allowing any bad usage "hidden". Added a test for this.


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


================
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
----------------
jhenderson wrote:
> Probably need to state why it can't be updated here.
Updated the error message.


================
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("=");
----------------
jhenderson wrote:
> Do you need the explicit `llvm::`?
Nope


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112116