[PATCH] D78005: [yaml2obj] - Reimplement how tool calculates file and memory sizes of segments.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 16 02:49:29 PDT 2020
jhenderson added inline comments.
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:756-761
+ if (!std::is_sorted(Fragments.begin(), Fragments.end(),
+ [](const Fragment &A, const Fragment &B) {
+ return A.Offset < B.Offset;
+ }))
+ reportError("sections in the program header with index " +
+ Twine(PhdrIdx) + " are not sorted by the file offset");
----------------
Perhaps worth splitting this into it's own patch. I think it's useful, but it seems to be unrelated to fixing address calculations.
Also "by their file offsets"
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:758
+ [](const Fragment &A, const Fragment &B) {
+ return A.Offset < B.Offset;
+ }))
----------------
I'm concerned that if there are multiple sections with the same offset but different sizes, this doesn't enforce a particular ordering. I'm thinking multiple trailing SHT_NOBITS sections here again. There needs to be one or more fallbacks, probably the section address at least.
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:776-779
+ uint64_t FileSize = Fragments.back().Offset - PHeader.p_offset;
+ if (Fragments.back().Type != llvm::ELF::SHT_NOBITS)
+ FileSize += Fragments.back().Size;
+ PHeader.p_filesz = FileSize;
----------------
This doesn't look right to me. What if there are multiple SHT_NOBITS sections at the end of a segment?
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:788-802
+ uint64_t MemSize = Fragments.front().Offset - PHeader.p_offset;
+ for (size_t I = 0, E = Fragments.size(); I != E; ++I) {
+ const Fragment &Cur = Fragments[I];
+ const Fragment *Next =
+ (I + 1 < Fragments.size()) ? &Fragments[I + 1] : nullptr;
+
+ // The distance between the current and the next section can
----------------
This logic seems awfully complicated for what it's doing. If we enforce an address order as well as offset order (see above), then it can just be the relative offset of the last section within the segment plus the section size, i.e. p_offset - sh_offset + sh_size.
By the way, p_memsz should only be set if the segment contains allocatable segments. If it doesn't, I think it should be 0. That might be slightly tangential to this patch though, so could be a different one.
================
Comment at: llvm/test/Object/invalid.test:488
Machine: EM_X86_64
-Sections:
- - Name: .dynamic
----------------
Why has this test changed?
================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-notes.test:168
Machine: EM_X86_64
-Sections:
- - Name: .note
----------------
Why has this test changed?
================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:450
+## Case 3: the .bar section is placed before the .zed section in the file,
+## but the the sh_offset of .zed is less than the sh_offset of
+## the .bar section because of the "ShOffset" property.
----------------
Double "the".
================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:452
+## the .bar section because of the "ShOffset" property.
+## Document we report an error for such case.
+ - Type: PT_LOAD
----------------
"Document" implies to me that we are just showing the current behaviour and that it doesn't matter if it changes. I don't think that's right. It should be "Check that". Also "such a case".
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:3
-!ELF
+## Set the file offset of the first segment to 0x0
+# RUN: yaml2obj %s --docnum=1 -DOFFSET=0x0 -o %t1
----------------
This sets more than just the file offset of the first segment. It sets it for all segments. I think you need to modify the comment.
Also, missing trailing full stop.
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:12
+
+## Show the layout produced.
+# COMMON: Section Headers:
----------------
I think this comment is redundant. It's evident that we are checking the section and program headers.
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:16
+# COMMON-NEXT: [ 0] NULL 0000000000000000 000000 000000 00 0 0 0
+# COMMON-NEXT: [ 1] .nobits1 NOBITS 0000000200000000 000158 000010 00 WA 0 0 0
+# COMMON-NEXT: [ 2] .progbits1 PROGBITS 0000000200000010 000158 000001 00 WA 0 0 0
----------------
You could probably make things simpler by deleting the last 5 columns (keeping Address, Offset and Size).
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:20
+# COMMON-NEXT: [ 4] .nobits2 NOBITS 0000000200000030 000170 000020 00 WA 0 0 0
+## The .filler chuck (size = 0x90) is placed here.
+# COMMON-NEXT: [ 5] .nobits3 NOBITS 00000002000000e0 000200 0001d0 00 WA 0 0 0
----------------
chuck -> chunk
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:26
+# COMMON: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
+## This segment includes sections from 1 to 6.
+# NOOFFSET-NEXT: LOAD 0x000000 0x0000000200000000 0x0000000200000000 0x000200 0x000500 RW 0x10
----------------
Here and below, no need for "from".
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:27
+## This segment includes sections from 1 to 6.
+# NOOFFSET-NEXT: LOAD 0x000000 0x0000000200000000 0x0000000200000000 0x000200 0x000500 RW 0x10
+# OFFSET-NEXT: LOAD 0x000158 0x0000000200000000 0x0000000200000000 0x0000a8 0x0003a8 RW 0x10
----------------
Similar to above: the flags and alignment seem unrelated to the test and can probably be deleted.
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:32
+# OFFSET-NEXT: LOAD 0x000158 0x0000000200000000 0x0000000200000000 0x0000a8 0x0002a8 RW 0x10
+## This segment includes sections from 1 to 4 + .filler chunk.
+# NOOFFSET-NEXT: LOAD 0x000000 0x0000000200000000 0x0000000200000000 0x000200 0x000230 RW 0x10
----------------
+ the .filler chunk
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:35
+# OFFSET-NEXT: LOAD 0x000158 0x0000000200000000 0x0000000200000000 0x0000a8 0x0000d8 RW 0x10
+## This segment includes first two sections.
+# NOOFFSET-NEXT: LOAD 0x000000 0x0000000200000000 0x0000000200000000 0x000159 0x000169 RW 0x1
----------------
includes the first
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:49
ProgramHeaders:
- - Type: PT_LOAD
- Flags: [ PF_R ]
+## The segment that has all the sections included and has
+## two SHT_PROGBITS sections at the end.
----------------
The -> A
also below in all similar comments.
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:63
+ Offset: [[OFFSET]]
+## The segment with SHT_NOBITS sections has a single
+## SHT_PROGBITS section at the end.
----------------
has a -> and a
Here and below
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:88
+ Offset: [[OFFSET]]
+## The segment with SHT_NOBITS section has a
+## SHT_PROGBITS section at the end.
----------------
with -> with a
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:97
+ Offset: [[OFFSET]]
+## The segment has only a SHT_NOBITS section included.
+ - Type: PT_LOAD
----------------
No need for "included"
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:113
+## It is followed by two SHT_PROGBITS sections. Note that the virtual
+## address of the .progbits2 is aligned so that there is an alignment gap
+## between .progbits1 and .progbits2 sections.
----------------
of the -> of
================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:123
+ AddressAlign: 0x10
+## We have a SHT_NOBITS section at the middle of the segment.
+ - Name: .nobits2
----------------
at the -> in the
I'd delete "of teh segment" from here and elsewhere, since there are multiple segments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78005/new/
https://reviews.llvm.org/D78005
More information about the llvm-commits
mailing list