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

Mon Nov 8 16:55:54 PST 2021

leonardchan added a comment.

> I think we can leave the segment size, to avoid having to change segments in general (I could see an argument for shrinking or even possibly growing a segment, if the section is last, but I don't think we want to go down that route in this patch). I do think it's important to change the section size, though, in my opinion. You can just leave behind the old data in the part of the segment that is no longer covered by a section header, or do what happens when a section is explicitly removed from a segment.

Done.

> Now that I think about it, there should be nothing stopping you growing a section that isn't in a segment - llvm-objcopy should be able to happily update the other section offsets. I think we should only reject growing sections if they are part of a segment.

Done. Added a test case for this.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:3
+
+# Update the segment section with a regular chunk of data the same size as the section.
+# RUN: echo -n 11112222 > %t.text
----------------
jhenderson wrote:
> Nit: Newer tests in the llvm-binutils use `##` for comments, to help them stand out from RUN and CHECK lines.
> 
> Should we also have a test a case without a segment?
Done. Also added a case with a section not part of a segment.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:8
+
+# Update to original contents after another update should give us the original result.
+# RUN: echo -n 00000000 > %t2.text
----------------
jhenderson wrote:
> Sorry, you've completely lost me with this comment. I don't know what this case is trying to test.
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.

Perhaps I can just add this explanation as the comment.

================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:68
+    LastSec: .init
+  - Type: PT_LOAD
+    Flags: [ PF_R ]
----------------
jhenderson wrote:
> Why do we need 2 program headers?
This was part of leftovers from D59351. Removed since it looks like we don't need them either.

================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:558-559
+  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr = MemoryBuffer::getFile(File);
+  if (!BufOrErr)
+    return createFileError(File, errorCodeToError(BufOrErr.getError()));
+  std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
----------------
jhenderson wrote:
> Is there existing testing for this case?
There should be. This isn't new code and was just moved out of `handleArgs`.

================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:695
+
+  for (const auto &Flag : Config.UpdateSection) {
+    auto UpdateSection = [&](StringRef Name, ArrayRef<uint8_t> Data) {
----------------
jhenderson wrote:
> Nit: here (and in the existing code above for AddSection), I wouldn't use `auto`. If I'm not mistaken, `Flag` is a `StringRef`, meaning that the `const &` is pointless (`StringRef` is designed for passing by value after all), but the `auto` hides this fact.
> 
> Unrelated to that: I wonder if we should have a test case where a user sepcifies the same section for both --add-section and --update-section. To me, the logical behaviour is add it and then use the update section contents, regardless of the command-line option order, but really it should just be whatever GNU objcopy does. The same possibly applies with --remove-section, although I think you probably only need the first test, because we already test that --remove-section and --add-section do what you might expect.
Added cases for working with `--add-section`.  I haven't experimented too deeply with `--add-section` + `--update-section` on GNU objcopy, but it looks like it's functionally equivalent to just adding a section, then updating it normally as if it were an already existing section.

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2110-2115
+  for (auto it = Obj.getUpdatedSections().begin();
+       it != Obj.getUpdatedSections().end(); ++it) {
+    auto *Sec = it->first;
+    auto &Data = it->second;
+
+    auto *Parent = Sec->ParentSegment;
----------------
jhenderson wrote:
> The LLVM style guide states that we don't use `auto` unless the type is obvious from the immediate context, or for iterators (see https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable). I think the only `auto` that should be present in this block is the one in the for loop. But then, why is this a traditional style for loop rather than a range-based for loop?
I think I've grown a habit of reserving range-based for loops only for vector-like structures and using traditional loops for everything else. Updated.

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2116
+    auto *Parent = Sec->ParentSegment;
+    assert(Parent && "This section should've been part of a segment.");
+    uint64_t Offset =
----------------
jhenderson wrote:
> This assert will fire if you specify --update-section with a section that isn't in a segment, if I'm not mistaken.
Ah, thanks for catching. I believe this shouldn't fire now that we handle sections not part of segments.

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1024
   std::vector<SecPtr> RemovedSections;
+  DenseMap<SectionBase *, SmallVector<uint8_t, 16>> UpdatedSections;

----------------
jhenderson wrote:
> I don't think a `SmallVector` here is necessarily that sensible: I expect the majority of section content updates to be non-trivial in size, so 16 seems quite small. It may be better to just use `std::vector` (but I could be persuaded otherwise).
That could be the case. I've personally never used `--update-section` so I don't know how many times/sections someone will update in a single invocation. Our specific use case I believe is to just update a single `.checksum` section.

Regardless, changed to `std::vector`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112116