[yaml2obj] - Program headers: add an additional check for `Offset`

[grimar]

Summary

The Offset field is used to set the file offset of a program header.
In a normal object it should not be greater than the minimal offset
of sections included into segment.

This patch adds a check for that and adds tests.

[grimar]

llvm/test/tools/llvm-objcopy/ELF/preserve-segment-contents.test

348 ↗

(On Diff #258062)

I think for empty segments we can allow any "Offset". Then similar changes will go away.

[grimar]

• Allow any "Offset" for empty segments.

[jhenderson]

Not looked at the detail of this change yet, but I'm wondering why we need it? One way of looking at things is that a segment is just a range. It doesn't have any literal contents in the sense of things it strictly owns, unlike sections which need to have contents placed at specific locations, which cannot overlap other contents of the file. Consequently, we essentially need two offset fields for sections: 1) where the contents are placed (derived implicitly), and 2) what the sh_offset field should be (allowing it to be placed in places where contents otherwise couldn't be written).

Can't the regular Offset field be used for this instead? Yes, that might mean that the offset is greater than the minimum section offset, but I think that's okay?

[grimar]

In D78304#1991775, @jhenderson wrote:

Can't the regular Offset field be used for this instead? Yes, that might mean that the offset is greater than the minimum section offset, but I think that's okay?

That is what I was not sure is OK and why this patch appeared. We have a few test cases like:

Sections:
  - Name: .dynamic
    Type: SHT_DYNAMIC
    Entries:
      - Tag:   DT_NULL
        Value: 0
ProgramHeaders:
  - Type:    PT_DYNAMIC
    Offset: 0xffff0000
    Sections:
      - Section: .dynamic

I read it as (knowing the internal logic): "take the list of 'Sections` and assign the proper memory size, file size and the offset.
But since the "Offset" is specified, it itself reads as "and change the offset to something", what can be valid because we might need the offset to be lower than the minimum section offset.
But when it is not lower its meaning intersects with the section list. It looks like an issue because it is not clear what the correct logic should be probably? E.g. which memory and file size we want to set?
Using of "POffset" here would say: "take the list of sections, calculate everything as usual, but override the final offset"

Does it make sense?

[jhenderson]

In D78304#1991909, @grimar wrote:

Does it make sense?

I think the problem here is that we have two competing sources of information for how to layout a program header - the Sections list and the Offset key. Possibly me mental model is outdated, but I thought the layout of the program headers had no effect on that of the sections? In other words, I thought that a section offset was based purely on its alignment and the positions and sizes of any sections/fillers/headers before it? Assuming that my model is correct, a program header's offset and size are determined by the contents, if unspecified, but a forced Offset greater than the start offset of some section seems invalid, and should therefore generate an error?

But when it is not lower its meaning intersects with the section list. It looks like an issue because it is not clear what the correct logic should be probably? E.g. which memory and file size we want to set?

In other words, don't try to derive a memory and file size and just emit an error in this case. It doesn't need to be allowed to create a broken ELF in this instance, because you can always specify the sizes explicitly.

The alternative of course is to completely change yaml2obj's model to lead with program headers, i.e. sections listed in program headers should be placed based on the offset of the program header. I don't think this is necessarily a good change to make though.

[grimar]

There is some misunderstanding it seems:

In D78304#1992107, @jhenderson wrote:

In D78304#1991909, @grimar wrote:

Does it make sense?

I think the problem here is that we have two competing sources of information for how to layout a program header - the Sections list and the Offset key. Possibly me mental model is outdated, but I thought the layout of the program headers had no effect on that of the sections? In other words, I thought that a section offset was based purely on its alignment and the positions and sizes of any sections/fillers/headers before it?

It is correct.

but a forced Offset greater than the start offset of some section seems invalid, and should therefore generate an error?

This is what this patch does. It introduces an error when the program header Offset forced is greater than the minimal section offset.
But since we need a way to set an arbitrary value for some tests (to override p_offset), I've introduced POffset, which does not affect on the properties of a program header, it just overrides the p_offset field without any checks.

So I think the behavior you described is exactly what this patch does, or not ?

[jhenderson]

In D78304#1992157, @grimar wrote:

This is what this patch does. It introduces an error when the program header Offset forced is greater than the minimal section offset.
But since we need a way to set an arbitrary value for some tests (to override p_offset), I've introduced POffset, which does not affect on the properties of a program header, it just overrides the p_offset field without any checks.

So I think the behavior you described is exactly what this patch does, or not ?

The bit I don't understand is the need for the POffset field. In such instances, just don't list the sections and use explicit Offset + FileSize/MemSize. The sizes wouldn't make sense anyway since the program header would be at a weird location.

[grimar]

In D78304#1992196, @jhenderson wrote:

In D78304#1992157, @grimar wrote:

This is what this patch does. It introduces an error when the program header Offset forced is greater than the minimal section offset.
But since we need a way to set an arbitrary value for some tests (to override p_offset), I've introduced POffset, which does not affect on the properties of a program header, it just overrides the p_offset field without any checks.

So I think the behavior you described is exactly what this patch does, or not ?

The bit I don't understand is the need for the POffset field. In such instances, just don't list the sections and use explicit Offset + FileSize/MemSize. The sizes wouldn't make sense anyway since the program header would be at a weird location.

OK, great! That what I did it initially in D78005 (in tests) and it sounds good to me. Then I'll remove the "POffset" field from this patch. So it will just introduce the offset validation check and adopt existent test cases.

[grimar]

• Remove "POffset".

[jhenderson]

llvm/lib/ObjectYAML/ELFEmitter.cpp

760

Should this ignore SHT_NOBITS?

764

I think it's okay to slightly simplify to make the error message slightly less verbose:

"'Offset' for segment with index"

766

I think this needs rewording:

" must be less than or equal to the minimum file offset of all included sections (0x123456)"

llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml

95

This comment is stale and needs updating.

113

minimal section offset -> minimum offset

(I would generally prefer minimum to minimal as minimal can also just mean "really small", so could occasionally be ambiguous)

143

FWIW, I wouldn't indent PT_LOAD here or below. I don't consider the Sections block as important for spacing, becuase it doesn't have a single inline value.

151

I would change "when it is less or equal to" to "to be less than or equal to".

Also, this test case only shows one of "equal to" or "less than". It should probably test both cases.

180

Perhaps worth also showing that ShOffset is ignored for these calculations (or not as the case may be)?

[jhenderson]

BTW, the title of the patch seems to have gotten messed up.

[grimar]

• Addressed review comments.

llvm/lib/ObjectYAML/ELFEmitter.cpp

760

I am not sure why it should..
Anyways this patch should not change the logic of computations. It's intended to add an Offset check only.

(aside: this piece was changed after rebasing, now when we know that frarments are sorted, the code is simpler).

llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml

113

Ok, thanks.

180

Done.

[grimar]

• Fix comment.

[jhenderson]

LGTM, with nit.

llvm/lib/ObjectYAML/ELFEmitter.cpp

760

I am not sure why it should..
Anyways this patch should not change the logic of computations. It's intended to add an Offset check only.

(aside: this piece was changed after rebasing, now when we know that frarments are sorted, the code is simpler).

Not sure where I was coming form with that comment before, but it's worth noting that SHT_NOBITS offsets in theory should be meaningless. I don't see a strong reason to ignore them though.

llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml

186

Nit: please add an extra space before FileCheck.

llvm/include/llvm/ObjectYAML/ELFYAML.h

struct ProgramHeader {
   ELF_PF Flags;
   llvm::yaml::Hex64 VAddr;
   llvm::yaml::Hex64 PAddr;
   Optional<llvm::yaml::Hex64> Align;
   Optional<llvm::yaml::Hex64> FileSize;
   Optional<llvm::yaml::Hex64> MemSize;
   Optional<llvm::yaml::Hex64> Offset;
   std::vector<SectionName> Sections;
+
+  // This can be used to override the offset stored in the p_offset field.
+  Optional<llvm::yaml::Hex64> POffset;
 };
 
 struct Symbol {
   StringRef Name;
   ELF_STT Type;
   StringRef Section;
   Optional<ELF_SHN> Index;
   ELF_STB Binding;

 
 template <>
 struct MappingTraits<ELFYAML::FileHeader> {
   static void mapping(IO &IO, ELFYAML::FileHeader &FileHdr);
 };
 
 template <> struct MappingTraits<ELFYAML::ProgramHeader> {
   static void mapping(IO &IO, ELFYAML::ProgramHeader &FileHdr);
+  static StringRef validate(IO &IO, ELFYAML::ProgramHeader &FileHdr);
 };
 
 template <>
 struct MappingTraits<ELFYAML::Symbol> {
   static void mapping(IO &IO, ELFYAML::Symbol &Symbol);
   static StringRef validate(IO &IO, ELFYAML::Symbol &Symbol);
 };
 

llvm/lib/ObjectYAML/ELFEmitter.cpp

 template <class ELFT>
 void ELFState<ELFT>::setProgramHeaderLayout(std::vector<Elf_Phdr> &PHeaders,
                                             std::vector<Elf_Shdr> &SHeaders) {
   uint32_t PhdrIdx = 0;
   for (auto &YamlPhdr : Doc.ProgramHeaders) {
     Elf_Phdr &PHeader = PHeaders[PhdrIdx++];
     std::vector<Fragment> Fragments = getPhdrFragments(YamlPhdr, SHeaders);
 
-    if (YamlPhdr.Offset) {
-      PHeader.p_offset = *YamlPhdr.Offset;
-    } else {
-      if (YamlPhdr.Sections.size())
-        PHeader.p_offset = UINT32_MAX;
-      else
-        PHeader.p_offset = 0;
-
-      // Find the minimum offset for the program header.
-      for (const Fragment &F : Fragments)
-        PHeader.p_offset = std::min((uint64_t)PHeader.p_offset, F.Offset);
+    uint64_t PhdrFileOffset = Fragments.empty() ? 0 : UINT64_MAX;
+    // Find the minimum section file offset.
+      PhdrFileOffset = std::min(PhdrFileOffset, F.Offset);
+
+    if (YamlPhdr.Offset) {
+      if (!Fragments.empty() && *YamlPhdr.Offset > PhdrFileOffset)
+        reportError("the 'Offset' specified for the segment with index " +
+                    Twine(PhdrIdx) +
+                    " must be less or equal than the minimal file offset of a "
+                    "section included into it (0x" +
+                    Twine::utohexstr(PhdrFileOffset) + ")");
+      PhdrFileOffset = *YamlPhdr.Offset;
     }
+    PHeader.p_offset =
+        YamlPhdr.POffset ? uint64_t(*YamlPhdr.POffset) : PhdrFileOffset;
 
     // Find the maximum offset of the end of a section in order to set p_filesz
     // and p_memsz. When setting p_filesz, trailing SHT_NOBITS sections are not
     // counted.
-    uint64_t FileOffset = PHeader.p_offset, MemOffset = PHeader.p_offset;
+    uint64_t FileOffset = PhdrFileOffset, MemOffset = PhdrFileOffset;
     for (const Fragment &F : Fragments) {
       uint64_t End = F.Offset + F.Size;
       MemOffset = std::max(MemOffset, End);
 
       if (F.Type != llvm::ELF::SHT_NOBITS)
         FileOffset = std::max(FileOffset, End);
     }
 
     // Set the file size and the memory size if not set explicitly.
     PHeader.p_filesz = YamlPhdr.FileSize ? uint64_t(*YamlPhdr.FileSize)
-                                         : FileOffset - PHeader.p_offset;
+                                         : FileOffset - PhdrFileOffset;
     PHeader.p_memsz = YamlPhdr.MemSize ? uint64_t(*YamlPhdr.MemSize)
-                                       : MemOffset - PHeader.p_offset;
+                                       : MemOffset - PhdrFileOffset;
 
     if (YamlPhdr.Align) {
       PHeader.p_align = *YamlPhdr.Align;
     } else {
       // Set the alignment of the segment to be the maximum alignment of the
       // sections so that by default the segment has a valid and sensible
       // alignment.
       PHeader.p_align = 1;

llvm/lib/ObjectYAML/ELFYAML.cpp

void MappingTraits<ELFYAML::ProgramHeader>::mapping(
   IO.mapOptional("Flags", Phdr.Flags, ELFYAML::ELF_PF(0));
   IO.mapOptional("Sections", Phdr.Sections);
   IO.mapOptional("VAddr", Phdr.VAddr, Hex64(0));
   IO.mapOptional("PAddr", Phdr.PAddr, Phdr.VAddr);
   IO.mapOptional("Align", Phdr.Align);
   IO.mapOptional("FileSize", Phdr.FileSize);
   IO.mapOptional("MemSize", Phdr.MemSize);
   IO.mapOptional("Offset", Phdr.Offset);
+
+  // obj2yaml does not dump these fields. They are expected to be empty when we
+  // are producing YAML, because yaml2obj sets appropriate values for them
+  // automatically when they are not explicitly defined.
+  assert(!IO.outputting() || !Phdr.POffset.hasValue());
+  IO.mapOptional("POffset", Phdr.POffset);
+}
+
+StringRef
+MappingTraits<ELFYAML::ProgramHeader>::validate(IO &IO,
+                                                ELFYAML::ProgramHeader &Phdr) {
+  if (Phdr.POffset && Phdr.Offset)
+    return "POffset and Offset cannot both be specified for ProgramHeader";
+  return StringRef();
 }
 
 LLVM_YAML_STRONG_TYPEDEF(StringRef, StOtherPiece)
 
 template <> struct ScalarTraits<StOtherPiece> {
   static void output(const StOtherPiece &Val, void *, raw_ostream &Out) {
     Out << Val;
   }

llvm/test/Object/invalid.test

FileHeader:
   Machine: EM_X86_64
 Sections:
   - Name: .dynamic
     Type: SHT_DYNAMIC
     Entries:
       - Tag:   DT_NULL
         Value: 0
 ProgramHeaders:
-  - Type:   PT_DYNAMIC
-    Offset: 0xffff0000
+  - Type:    PT_DYNAMIC
+    POffset: 0xffff0000
     Sections:
       - Section: .dynamic
 
 ## PT_DYNAMIC's p_filesz field is so large that p_offset + p_filesz is larger
 ## than the object size. Check llvm-readobj reports it.
 
 # RUN: yaml2obj %s --docnum=23 -o %t23
 # RUN: llvm-readobj --dyn-relocations %t23 2>&1 \

llvm/test/tools/llvm-objcopy/ELF/invalid-p_filesz-p_offset.test

FileHeader:
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
   - Name: .foo
     Type: SHT_PROGBITS
     Size: 1
 ProgramHeaders:
   - Type:        PT_LOAD
-    Offset:      0x100000
+    POffset:     0x100000
     FileSize:    1
     Sections:
       - Section: .foo

llvm/test/tools/llvm-readobj/ELF/gnu-notes.test

FileHeader:
   Data:    ELFDATA2LSB
   Type:    ET_CORE
   Machine: EM_X86_64
 Sections:
   - Name:  .note
     Type:  SHT_NOTE
     Notes: []
 ProgramHeaders:
-  - Type: PT_NOTE
-    Offset: 0xffff0000
+  - Type:    PT_NOTE
+    POffset: 0xffff0000
     Sections:
       - Section: .note
 
 ## Test tools report an error if a note program header has an invalid size that
 ## goes past the end of file.
 
 # RUN: yaml2obj --docnum=5 %s -o %t5.so
 # RUN: not llvm-readelf --notes %t5.so 2>&1 | FileCheck -DFILE=%t5.so  %s --check-prefix=ERR4

llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml

 ## Show that yaml2obj properly emits program headers with explicit file size,
 ## memory size and offset parameters.
 
-# RUN: yaml2obj %s -o %t
-# RUN: llvm-readobj %t --program-headers | FileCheck %s
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj %t1 --program-headers | FileCheck %s
 
 # CHECK: ProgramHeaders [
 # CHECK:    Offset: 0x1234
 # CHECK:    FileSize: 1111
 # CHECK:    MemSize: 9999
 
 # CHECK:    Offset: 0x2000
 # CHECK:    FileSize: 6

 # CHECK:    FileSize: 3
 # CHECK:    MemSize: 2
 
 # CHECK:    Offset: 0x2004
 # CHECK:    FileSize: 4
 # CHECK:    MemSize: 6
 # CHECK: ]
 
-!ELF
+--- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:
   - Name: .text
     Type: SHT_PROGBITS
Sections:
       - Section: .rodata
   # Program header with sections, valid properties.
   - Type:     0x6abcdef0
     Offset:   0xffe
     FileSize: 7
     MemSize:  9
     Sections:
       - Section: .text
   # Program header with sections, invalid properties.
   - Type:     0x6abcdef0
-    Offset:   0x3000
+    POffset:  0x3000
     FileSize: 3
     MemSize:  2
     Sections:
       - Section: .data
   # Program header with 2 SHT_NOBITS sections.
   - Type:     0x6abcdef0
     Offset:   0x2004
     Sections:
       - Section: .data
       - Section: .nobits1
       - Section: .nobits2
+
+## Test "Offset" and "POffset" properties.
+
+## Check we report an error when the "Offset" and "POffset" are used together.
+# RUN: not yaml2obj --docnum=2 %s -o %t3 2>&1 | FileCheck %s --check-prefix=BOTH
+
+# BOTH: error: POffset and Offset cannot both be specified for ProgramHeader
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+ProgramHeaders:
+  - Type:     PT_LOAD
+    POffset:  0x0
+    Offset:   0x0
+    Sections: []
+
+## Check that by default the segments p_offset field is set to the
+## offset of the section with the minimal section offset.
+
+# RUN: yaml2obj --docnum=3 -DOFFSET_KEY="" %s -o %t2
+# RUN: llvm-readelf %t2 --sections --program-headers | \
+# RUN:   FileCheck %s --check-prefixes=SEC,DEFAULT
+
+# SEC: [Nr] Name Type     Address          Off
+# SEC: [ 1] .foo PROGBITS 0000000000001000 000078
+
+# DEFAULT: Type Offset
+# DEFAULT: LOAD 0x000078
+
+## Check we can set the "Offset" value explicitly when it is less or equal to
+## the offset of a section in the segment.
+# RUN: yaml2obj --docnum=3 -DOFFSET_KEY="Offset: 0x78" %s -o %t3
+# RUN: llvm-readelf %t3 --sections --program-headers | \
+# RUN:   FileCheck %s --check-prefixes=SEC,DEFAULT
+
+## Check we report an error when the "Offset" value is larger than the offset of a section in the segment.
+# RUN: not yaml2obj --docnum=3 -DOFFSET_KEY="Offset: 0x79" %s -o /dev/null 2>&1 | \
+# RUN:  FileCheck %s --check-prefix=INVALID-OFFSET
+
+# INVALID-OFFSET: yaml2obj: error: the 'Offset' specified for the segment with index 1 must be less or equal than the minimal file offset of a section included into it (0x78)
+
+## We can use "POffset" to set an arbitrary offset for a segment.
+# RUN: yaml2obj --docnum=3 -DOFFSET_KEY="POffset: 0x79" %s -o %t4
+# RUN: llvm-readelf %t4 --sections --program-headers | \
+# RUN:   FileCheck %s --check-prefixes=SEC,POFFSET1
+
+# POFFSET1: Type Offset
+# POFFSET1: LOAD 0x000079
+
+# RUN: yaml2obj --docnum=3 -DOFFSET_KEY="POffset: 0xFF112233" %s -o %t5
+# RUN: llvm-readelf %t5 --sections --program-headers | \
+# RUN:   FileCheck %s --check-prefixes=SEC,POFFSET2
+
+# POFFSET2: Type Offset
+# POFFSET2: LOAD 0xff112233
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    .foo
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC ]
+    Size:    0x1
+    Address: 0x1000
+  - Name:    .bar
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC ]
+    Size:    0x1
+ProgramHeaders:
+  - Type:    PT_LOAD
+    [[OFFSET_KEY]]
+    Sections:
+      - Section: .foo
+      - Section: .bar