[PATCH] D67689: [llvm-objcopy] Add support for --gap-fill and --pad-to options
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 13 00:58:34 PST 2020
jhenderson added reviewers: MaskRay, grimar.
jhenderson added a comment.
I'm out of time to review this further, but there's plenty of comments to take a look at. Also adding a couple more reviewers who might be able to help out.
================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:84
+ Fill gaps between two adjacent loadable sections with the value ``<value>``.
+ The value will be truncated to 8 bits or 0xff if larger than 64-bit integer.
+
----------------
Presumably you meant bytes, not bits?
================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:109-110
+
+ Pad the last loadable section to ``<address>`` using a value of zero or
+ the value specified by :option:`--gap-fill`.
+
----------------
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:4
+## or between sections serving as 'gaps'. Check that these gaps are filled
+## with the value specified by the --gap-fill option except the gaps at the
+## start of the first allocable section (.space1) which is preserved.
----------------
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:7
+
+## Section/Segment layout by yaml
+##
----------------
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:21
+
+# RUN: yaml2obj %s > %t
+
----------------
This tends to be preferred in newer tests.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:23
+
+## Verify section headers before we perform several testings.
+# RUN: llvm-readelf -S %t | FileCheck %s --check-prefix=ORG-SHDR
----------------
I'm not convinced this sanity test is worthwhile, since that just shows that yaml2obj or llvm-readelf are broken, whereas this is a test for llvm-objcopy.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:35-36
+
+# RUN: not llvm-objcopy --gap-fill %t 2>&1 | FileCheck %s --check-prefix=EMPTY
+# EMPTY: no input file specified
+
----------------
You might also want a variant of this test with --gap-fill last.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:47
+
+## Verify no error since truncating happens
+# RUN: llvm-objcopy --gap-fill=0x1122 %t %t-val16
----------------
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:48
+## Verify no error since truncating happens
+# RUN: llvm-objcopy --gap-fill=0x1122 %t %t-val16
+# RUN: llvm-objcopy --gap-fill=0x112233445566778899aabbccddeeff00cccc %t %t-llvalue
----------------
What's the point of this line?
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:49
+# RUN: llvm-objcopy --gap-fill=0x1122 %t %t-val16
+# RUN: llvm-objcopy --gap-fill=0x112233445566778899aabbccddeeff00cccc %t %t-llvalue
+
----------------
It's not clear in what way this is truncated. You need to show the actual behaviour here too. You could do this by picking a value that truncates to the same value as a "normal" test and then using `cmp` to show that the output is identical. The "normal" test output will still need verifying to show the right output has been emitted.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:54-55
+## Verify the headers are expected.
+# RUN: llvm-readelf -S %t-filled | FileCheck %s --check-prefix=SHDR
+# RUN: llvm-readelf -l %t-filled | FileCheck %s --check-prefix=PHDR
+
----------------
And then change `SHDR`/`PHDR` below to `CHECK`. (Note: I can't remember whether program headers or section headers appear first - you may need to change the order of the two check blocks around to match).
It is not obvious from the comments in this test or the documentation that --gap-fill changes the size of sections. This is actually slightly strange behaviour to me. Are you sure it reflects what GNU objcopy does? If it is, please improve the documentation to say that the filling is achieved by padding the tail of sections, and probably also explain what is expected in the test comments.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:58-66
+# SHDR: [Nr] Name Type Address Off Size ES Flg Lk Inf Al
+# SHDR: [ 0] NULL 0000000000000000 000000 000000 00 0 0 0
+# SHDR: [ 1] .nogap PROGBITS 0000000000000102 000202 000006 00 A 0 0 1
+# SHDR: [ 2] .gap1 PROGBITS 0000000000000108 000208 000008 00 AX 0 0 1
+# SHDR: [ 3] .gap2 PROGBITS 0000000000000110 000210 0000f4 00 A 0 0 1
+# SHDR: [ 4] .nobit_tbss NOBITS 0000000000000200 000400 000018 00 WAT 0 0 8
+# SHDR: [ 5] .foo PROGBITS 0000000000000204 000404 000004 00 WA 0 0 1
----------------
The majority of this data is not relevant to the test. I suggest removing the columns that aren't important. For those at the start or end, you can simply omit them. For those in between columns you want to check (name, offset, size probably), you can just use `{{.*}}` to indicate a regex matching anything.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:68
+
+## The 'Offset' of segment1 appears to be wrong when using objcopy. Here we checkes the result of llvm-objcopy.
+# PHDR: Program Headers:
----------------
This comment appears to be referring to a potential problem with GNU objcopy? If so, it's not really relevant, and the comment could rot if the behaviour in GNU objcopy is fixed/changes.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:93
+# RUN: od -An -x -j 0x200 -N 0x2 %t-filled > %t-filled.space1
+# RUN: cmp %t.space1 %t-filled.space1
+
----------------
Don't you also need to check that the value is correct?
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:95
+
+## Verify all section gaps are filled with 0xe9. The verification is done by two steps:
+## First read the section original data and ensure they are not altered.
----------------
I think it would be a good idea to show how a multi-byte value is written too, especially in relation to being truncated if the gap size isn't a multiple of the specified value.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:99
+
+## Check all original data of .gap1 not altered (the first 7 bytes)
+# RUN: llvm-objcopy -j .gap1 -O binary %t %t.1
----------------
Make sure to add full stops to the end of all comments.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:118
+## Check the rest of .gap2 (0xf4 - 0x4, i.e. 240 bytes) are filled with 0xe9
+# RUN: od -v -An -t x1 -j 0x4 -N 240 %t-filled.2 | grep "e9" | wc -w | FileCheck %s --check-prefix=GAP2
+# GAP2: 240
----------------
Don't use `grep` in tests. See https://llvm.org/docs/TestingGuide.html#writing-new-regression-tests.
Could you just arrange your sections to be closer together so that you don't need such a large amount to check? You could then easily check using od.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:123-138
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_EXEC
+ Machine: EM_X86_64
+Sections:
+ - Name: .space1
+ Type: Fill
----------------
We usually try to remove excessive space between the key and value in YAML, to make it a little easier to read (the edit suggestion is just partial, the same sort of edit applies throughout). You also don't need this many fields - AddressAlign defaults to 0/1 (I forget which, but they are semantically the same here), Size is auto-derived from the Content if not specified, and Address is automatically calculated based on the segment VAddr, IIRC. Get rid of the excess here and below.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:141
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ Address: 0x0108
----------------
The SHF_EXECINSTR is not relevant to the test, so get rid of it to make the test simpler. Same goes below - the TLS bit is probably not relevant to the test (NOBITS might be on the other hand).
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:165
+ Size: 0x0018
+ - Name: .space4 # in the different segment
+ Type: Fill
----------------
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:186
+ EntSize: 0x0001
+ Content: 4743433A
+ProgramHeaders:
----------------
Do you actually need any content here? It's not clear what the purpose of this section is.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:188-194
+ - Type: PT_LOAD # Segment1 is nested in Seg2.
+ Flags: [ ]
+ VAddr: 0x0108
+ PAddr: 0x0108
+ Align: 0x0001
+ FirstSec: .gap1
+ LastSec: .gap1
----------------
This generates illegal ELF. The gABI dictates that PT_LOAD segments must be in ascending address order. Please move this segment accordingly.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/pad-to.test:1
+
+# RUN: yaml2obj %s > %t-yaml
----------------
Looks like this test is missing a comment explaining the purpose of the test.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/pad-to.test:2
+
+# RUN: yaml2obj %s > %t-yaml
+
----------------
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/pad-to.test:4
+
+## Use Sections/Segment layouts by llvm-objcopy instead of by yaml2obj.
+# RUN: llvm-objcopy %t-yaml %t
----------------
Why? (Please expand the comment to explain the reason)
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/pad-to.test:26
+# RUN: cmp %t %t-p1
+# RUN: llvm-objcopy --pad-to=0x200 %t %t-p2
+# RUN: cmp %t %t-p2
----------------
How about --pad-to=0x204 for testing the edge case?
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/pad-to.test:44-46
+## Second, check .foo is padded from 0x204-0x214 (16 bytes) with 0
+# RUN: od -v -An -t x1 -j 0x4 -N 16 %t-pad.foo | grep "00" | wc -w | FileCheck %s --check-prefix=DATA
+# DATA: 16
----------------
See my comment about not using grep in the --gap-fill test.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/pad-to.test:51
+
+## Verify .foo is padded as expected
+# RUN: llvm-objcopy -j .foo -O binary %t-gap-pad %t-gap-pad.foo
----------------
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/pad-to.test:53
+# RUN: llvm-objcopy -j .foo -O binary %t-gap-pad %t-gap-pad.foo
+# RUN: od -v -An -t x1 -j 0x4 -N 16 %t-gap-pad.foo | grep "cc" | wc -w | FileCheck %s --check-prefix=VAL
+# VAL: 16
----------------
As above - don't use grep.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/pad-to.test:57-59
+# RUN: llvm-readelf -h %t-pad | FileCheck %s --check-prefix=FHDR
+# RUN: llvm-readelf -S %t-pad | FileCheck %s --check-prefix=SHDR
+# RUN: llvm-readelf -l %t-pad | FileCheck %s --check-prefix=PHDR
----------------
This can be combined into a single command, as suggested in the other test.
================
Comment at: llvm/test/tools/llvm-objcopy/ELF/pad-to.test:76
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
----------------
Same comments as gap-fill.test re. reducing what's in the YAML to a more minimal amount.
================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:629
+ Arg->getValue());
+ // Truncation if needed.
+ if (Val.get().getBitWidth() > 64)
----------------
================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:630-631
+ // Truncation if needed.
+ if (Val.get().getBitWidth() > 64)
+ Config.GapFill = 0xff;
+ else
----------------
This needs testing.
================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:633
+ else
+ Config.GapFill = static_cast<uint8_t>(Val.get().getZExtValue());
+ }
----------------
Are you sure this is supposed to be truncated to a `uint8_t` and not say a 4-byte word or similar that is repeated?
================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:626-633
+ llvm::stable_sort(LoadableSections,
+ [](const SectionBase *LHS, const SectionBase *RHS) {
+ if (LHS->Addr < RHS->Addr)
+ return true;
+ if (LHS->Addr > RHS->Addr)
+ return false;
+ return (LHS->Size < RHS->Size);
----------------
You need testing to show why this sorting is necessary.
================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:647-650
+ uint8_t *N = new uint8_t[Sec->Size];
+ llvm::copy(Sec->OriginalData, &N[0]);
+ memset(N + Sec->OriginalData.size(), Value, GapsSize);
+ Sec->OriginalData = makeArrayRef<uint8_t>(N, Sec->Size);
----------------
This isn't the way to do this. A) as the name implies, `OriginalData` is intended to be immutable (maybe it should be const). Changing it changes llvm-objcopy's view of what the section originally had for data. B) This looks like a memory leak - you new an array and then create an ArrayRef to it, but there's nothing cleaning up the memory afterwards.
Take a look at `OwnedDataSection`. It's likely you'll want something like this. Alternatively, you could add a field to the `SectionBase` class called "AddedData" or something like that which is written after the rest of the section's data.
On a related note, beware of other section types like dynamic sections. I don't know if GNU objcopy changes the size of such sections. If it does, we'll need to too, but the logic for such sections is different in llvm-objcopy, so may need additional handling and testing.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67689/new/
https://reviews.llvm.org/D67689
More information about the llvm-commits
mailing list