[PATCH] D78927: [yaml2obj] - Introduce the "Offset" property for sections.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 13 00:30:15 PDT 2020
jhenderson added inline comments.
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1035-1036
+ if ((uint64_t)*Offset < CurrentOffset) {
+ reportError("the 'Offset' value (0x" +
+ Twine::utohexstr((uint64_t)*Offset) + ") goes backward");
+ return CurrentOffset;
----------------
grimar wrote:
> grimar wrote:
> > MaskRay wrote:
> > > grimar wrote:
> > > > MaskRay wrote:
> > > > > grimar wrote:
> > > > > > jhenderson wrote:
> > > > > > > I can see why you've done this, but I think there's also a use-case for a section header table where the sections aren't in offset order, which I believe would run into this problem. You could use the SHOffset field instead, but that has its own problems (doesn't place data at the specified location, shouldn't be included in program header calculations etc).
> > > > > > yaml2obj does not support writing section out of order currently. This patch has no intention to change this.
> > > > > > We always write sections in the same order they are listed in a YAML.
> > > > > >
> > > > > > To implement what you want perhaps we might want just to introduce another field, like "SectionHeaderIndex".
> > > > > > Or may be expand YAML format to it allow to describe section header table (and may be it's location) explicitly.
> > > > > `alignToOffset(sec)` can be called according to the section offset order in `Doc.Chunks` in `ELFState<ELFT>::initSectionHeaders` may be sufficient, but I haven't tried.
> > > > > alignToOffset(sec) can be called according to the section offset order in Doc.Chunks in ELFState<ELFT>::initSectionHeaders may be sufficient.
> > > >
> > > > Not sure I understand what you suggest? Calling of `alignToOffset` in `initSectionHeaders` is what this patch does already.
> > > Sorry for the confusion. I mean (in a future change), we may change the order ELFState<ELFT>::alignToOffset is called on each section. We call alignToOffset following the order of section offsets. This may allow out-of-order sh_offset fields in the section header table.
> > >
> > > https://reviews.llvm.org/D78422#1991831 says
> > >
> > > > FWIW in embedded systems it is quite common to have linker scripts where different OutputSections need to be copied to various disjoint memory locations and these are often not in order of ascending address.
> > What I do not like in idea of allowing disordered offsets is:
> > 1) It might open a worm can: it will be easy to create overlapping sections or something alike. We then might want the code to diagnose it,
> > but it sounds a bit too complex and probably not what we want.
> > 2) Out logic of creating program headers assumes that chunks are ordered by offsets. Changing this assumption will introduce an additional complexity there.
> >
> > I am thinking about the possible extending the YAML instead. E.g. we could have something like:
> >
> > ```
> > --- !ELF
> > FileHeader:
> > Class: ELFCLASS64
> > Data: ELFDATA2LSB
> > Type: ET_REL
> > Machine: EM_X86_64
> > SectionHeader:
> > Location: <location> ## before sections/after sections
> > Sections:
> > - .foo
> > - .bar
> > Sections:
> > - Name: .foo
> > Type: SHT_PROGBITS
> > Size: 0x8
> > ...
> > ```
> >
> > The benefit is that it is more flexible and should not touch the rest of the code.
> `SectionHeader` in my sample should not be a part of `FileHeader` probably. E.g:
>
> ```
> --- !ELF
> FileHeader:
> Class: ELFCLASS64
> Data: ELFDATA2LSB
> Type: ET_REL
> Machine: EM_X86_64
> Sections:
> - Name: .foo
> Type: SHT_PROGBITS
> Size: 0x8
> ...
> SectionHeader:
> Location: <location> ## before sections/after sections
> Sections:
> - .foo
> - .bar
> ```
Sounds reasonable, though I'd suggest a couple of minor changes: 1) Call it `SectionHeaders` or `SectionHeaderTable`; 2) Allow it to take an explicit `Offset` maybe, possibly instead of `Location`, any maybe even add special values for `Offset` like `AutoBefore`, and `AutoAfter` or something like that.
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:221
+ uint64_t alignToOffset(ContiguousBlobAccumulator &CBA, unsigned Align,
+ llvm::Optional<llvm::yaml::Hex64> Offset);
----------------
They're not going to be that common, but sh_addralign and p_align are both 64-bit values for ELF64, so `Align` should be `uint64_t` here.
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:453
+ // 0 when not explicitly requested.
+ bool IsFirstUndefSection = SecNdx == 0;
+ if (!IsFirstUndefSection || Sec->Offset)
----------------
Nit: There's a double space after the variable name.
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1032
+
+ AlignedOffset = *Offset;
+ } else {
----------------
Maybe worth a comment explicitly saying we ignore alignment when an explicit offset has been requested, just to show that it is 100% intentional.
================
Comment at: llvm/test/tools/yaml2obj/ELF/custom-null-section.yaml:205-206
+
+## Check that we can set an offset for the SHT_NULL section explicitly using the "Offset" key.
+## Check it affects the section header table offset.
+# RUN: yaml2obj --docnum=10 %s -DOFFSET=0x100 -o %t10
----------------
The offset of a SHT_NULL section probably shouldn't affect that of other sections, since it is supposed to be "inactive". Not sure if it should be part of this change, but we should have a test case for it.
================
Comment at: llvm/test/tools/yaml2obj/ELF/custom-null-section.yaml:207-210
+# RUN: yaml2obj --docnum=10 %s -DOFFSET=0x100 -o %t10
+# RUN: llvm-readelf --headers --sections %t10 | FileCheck %s --check-prefix=EXPLICIT-OFFSET-A
+# RUN: yaml2obj --docnum=10 %s -DOFFSET=0x200 -o %t11
+# RUN: llvm-readelf --headers --sections %t11 | FileCheck %s --check-prefix=EXPLICIT-OFFSET-B
----------------
Any particular reason these aren't one test, with two SHT_NULL sections?
================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:4
+## Show how the 'Offset' field key can be used.
+## Show that it might affect the file size.
+
----------------
This comment needs updating, since you're checking the section header table offset now.
================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:95
+
+## 176 < 288 (start of section headers in the case 1).
+# BOTH-A: Start of section headers: 176 (bytes into file)
----------------
"in Case 1" or "in the first case"
================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:98
+
+## Show that the 'Offset' sets the physical offset in a file and the `ShOffset`
+## only overrides the sh_offset value of the .foo section.
----------------
the 'Offset' field
the 'ShOffset' field
================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:113
+
+## Show that the 'Offset' sets the physical offset in file and `ShOffset`
+## only affects on the sh_offset value of the .foo section (overrides it).
----------------
Ditto.
================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:114
+## Show that the 'Offset' sets the physical offset in file and `ShOffset`
+## only affects on the sh_offset value of the .foo section (overrides it).
+# BOTH-B: [Nr] Name Type Address Off
----------------
only affects the
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78927/new/
https://reviews.llvm.org/D78927
More information about the llvm-commits
mailing list