[PATCH] D112116: [llvm-objcopy] Add --update-section
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 20 00:52:22 PDT 2021
jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.
High-level question: does GNU objcopy change the section size for sections that are updated?
Overall, this approach seems reasonable, but there are some issues.
================
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
----------------
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?
================
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
----------------
Sorry, you've completely lost me with this comment. I don't know what this case is trying to test.
================
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
----------------
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?
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:19
+
+# We can't update sections which don't exist
+# RUN: not llvm-objcopy --update-section=.nosection=%t.text %t3 %t-err1 2>&1 | FileCheck %s --check-prefix=ERR1
----------------
Nit: this and the next comment are missing trailing full stops.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:22
+
+# We can't update certain types of sections
+# RUN: not llvm-objcopy --update-section=.bss=%t.text %t3 %t-err3 2>&1 | FileCheck %s --check-prefix=ERR3
----------------
If there's more than one type, I think we should test each individual type. If there's only one, just state that particular type in the comments.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:25
+
+# Fail on trying to insert data larger than the existing section.
+# RUN: echo -n 111122223 > %t4.text
----------------
This seems like it should only be an issue for segment sections, right? Probably need to say that in the comment and then have a test case where it works for sections not in segments.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:38
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ Address: 0x1000
----------------
The flags aren't important for this test, so I'd delete them.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:40
+ Address: 0x1000
+ AddressAlign: 0x8
+ Content: "3030303030303030"
----------------
I don't think the AddressAlign is important either. Delete it.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:42
+ Content: "3030303030303030"
+ - Name: .init
+ Type: SHT_PROGBITS
----------------
This section doesn't appear to be used, so delete it.
If it's important to the test case either name it accordingly or add a comment.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:48
+ AddressAlign: 0x8
+ - Name: .data
+ Type: SHT_PROGBITS
----------------
This section doesn't appear to be used, so delete it.
If it's important to the test case either name it accordingly or add a comment.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:62
+ - Type: PT_LOAD
+ Flags: [ PF_X, PF_R ]
+ VAddr: 0x1000
----------------
`Flags` aren't used. Delete them.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:64
+ VAddr: 0x1000
+ PAddr: 0x1000
+ Align: 0x1000
----------------
`PAddr` isn't used. Delete it.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:65
+ PAddr: 0x1000
+ Align: 0x1000
+ FirstSec: .text
----------------
I don't believe this `Align` is used either, so delete it.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:68
+ LastSec: .init
+ - Type: PT_LOAD
+ Flags: [ PF_R ]
----------------
Why do we need 2 program headers?
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/update-section.test:159
+# ERR1: error: {{.*}}Section '.nosection' not found
+# ERR3: error: {{.*}}Section '.bss' can't be updated
+# ERR4: error: {{.*}}Cannot fit data of size 9 into section '.text' with size 8
----------------
What about ERR2?
================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:885-887
+ if (!ArgValue.contains('='))
+ return createStringError(errc::invalid_argument,
+ "bad format for --update-section: missing '='");
----------------
This check needs a test case.
================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:889-891
+ return createStringError(
+ errc::invalid_argument,
+ "bad format for --update-section: missing file name");
----------------
This check needs a test case.
================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:553
+handleUserSection(StringRef Flag,
+ std::function<Error(StringRef, ArrayRef<uint8_t>)> F) {
+ std::pair<StringRef, StringRef> SecPair = Flag.split("=");
----------------
Use `function_ref` instead of `std::function`.
================
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);
----------------
Is there existing testing for this case?
================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:695
+
+ for (const auto &Flag : Config.UpdateSection) {
+ auto UpdateSection = [&](StringRef Name, ArrayRef<uint8_t> Data) {
----------------
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.
================
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;
----------------
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?
================
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 =
----------------
This assert will fire if you specify --update-section with a section that isn't in a segment, if I'm not mistaken.
================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2143
+ if (It == Sections.end())
+ return createStringError(errc::invalid_argument, "Section '%s' not found.",
+ Name.str().c_str());
----------------
Here and with the other error messages, the LLVM coding standard says error messages should generally not start with a capital letter or end with a full stop. See https://llvm.org/docs/CodingStandards.html#error-and-warning-messages.
================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2145
+ Name.str().c_str());
+ if (!(*It)->isReplaceable())
+ return createStringError(errc::invalid_argument,
----------------
Perhaps pull `*It` into its own variable, as it's repeated 3 times, and the meaning isn't particularly clear, especially in isolation.
================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:497
void finalize() override;
+ bool isReplaceable() const override { return Type != ELF::SHT_NOBITS; }
};
----------------
I feel like this would be better named as `hasContents`. After all, that's really the defining feature of SHT_NOBITS sections. It should probably also include `SHT_NULL` sections, since they are "inactive" and so their values are meaningless.
================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1024
std::vector<SecPtr> RemovedSections;
+ DenseMap<SectionBase *, SmallVector<uint8_t, 16>> UpdatedSections;
----------------
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).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112116/new/
https://reviews.llvm.org/D112116
More information about the llvm-commits
mailing list