[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.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:
> 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.
> 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.

I think I may want to push back on this part since this feels like it would be testing against undefined behavior (by checking for a value that we don't explicitly set/guarantee). I think the user just wants to ensure that the smaller chunk is written (and the size is updated), but not necessarily what the remaining data is. It just so happens that under the hood, we do sequential memcpy's that result on the old data still being there, but end-to-end-behavior-wise, I think we don't want to explicitly test the implementation.

I see that the test comment actually points this out, which is my bad. So I'll update this.

(I'll also admit that I can't find an easy way to test this other than doing a hexdump and inspecting specific bits, which seems a bit complex :)

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

Added a test for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112116