[yaml2obj] - Allow overriding sh_offset field from the YAML.

[grimar]

Summary

Some of our test cases are using objects which
has sections with a broken sh_offset field.

There was no way to set it from YAML until this patch.

[jhenderson]

I'm not sure the correct behaviour for an explicit offset is clear-cut. I'd think that it causes the section contents to be written at the specified offset, not that it just writes an arbitrary value in sh_offset. I've had times when I want to put my sections at a specific offset, but could only do this with tricks to do with alignment or padding sections. I'd expect an Offset field would resolve this issue (note that this is what happens for ProgramHeaders already).

Unfortunately, I think this change conflicts with that desire, and I don't see any easy way of reconciling the differences.

lib/ObjectYAML/ELFYAML.cpp

915 ↗

(On Diff #206858)

This assert seems a bit out-of-place?

916 ↗

(On Diff #206858)

I'd call this "Offset" not "ShOffset", to better match the other fields.

[grimar]

In D63879#1560868, @jhenderson wrote:

I'm not sure the correct behaviour for an explicit offset is clear-cut. I'd think that it causes the section contents to be written at the specified offset, not that it just writes an arbitrary value in sh_offset. I've had times when I want to put my sections at a specific offset, but could only do this with tricks to do with alignment or padding sections. I'd expect an Offset field would resolve this issue (note that this is what happens for ProgramHeaders already).

Yes, I had the same concerns. But I think it is fine generally: at any time we still can introduce a new field that might have a behavior you describe.
Its name can be SecOffset. See: currently all YAML fields names we have do not match the ELF fields name directly. E.g. we have Name but it doesn't match sh_name.
In this patch I named the field ShOffset to match the sh_offset. We can have an agreement that if a field matches the ELF field name then its intention os to
override the field's value. It is in line with D63771 btw.

[jhenderson]

In D63879#1561995, @grimar wrote:

In D63879#1560868, @jhenderson wrote:

Yes, I had the same concerns. But I think it is fine generally: at any time we still can introduce a new field that might have a behavior you describe.
Its name can be SecOffset. See: currently all YAML fields names we have do not match the ELF fields name directly. E.g. we have Name but it doesn't match sh_name.
In this patch I named the field ShOffset to match the sh_offset. We can have an agreement that if a field matches the ELF field name then its intention os to
override the field's value. It is in line with D63771 btw.

Okay, I can buy that, but this needs clear comments in the code, stating that this does NOT place the section data at the specified offset.

test/tools/yaml2obj/elf-override-shoffset.yaml

1 ↗

(On Diff #206858)

I wonder if it would be worth somehow showing in a test case that this field doesn't place section data at the specified offset? What do you think? Relatedly, having an offset outside the file and showing that the file size doesn't increase would probably be worthwhile.

[grimar]

• Addressed review comments.

lib/ObjectYAML/ELFYAML.cpp

915 ↗

(On Diff #206858)

Yes, it was a bit incomplete. Fixed.

916 ↗

(On Diff #206858)

So seems we agreed "ShOffset" makes sence, I leaved it as is.

[MaskRay]

lib/ObjectYAML/ELFYAML.cpp

916 ↗

(On Diff #206858)

Since the fields are already spelled this way: Flags.. I'll not bother asking you to change that. ShOffset seems fine. It is better than Offset I think.

(I'd prefer unmangled elf.h names: sh_flags, sh_offset, etc)

[jhenderson]

lib/ObjectYAML/ELFYAML.cpp

915 ↗

(On Diff #207025)

I'm still not clear why this assert exists specifically for ShOffset, and not for the other fields. It needs commenting or some sort of message needs adding to the assert explaining it.

test/tools/yaml2obj/elf-override-shoffset.yaml

76 ↗

(On Diff #207025)

You don't need this rm here. The first od line will replace its contents.

[grimar]

• Addressed review comments.

[jhenderson]

LGTM, with the comment fix.

lib/ObjectYAML/ELFYAML.cpp

916 ↗

(On Diff #207049)

"when we are producing",
yam2obj -> yaml2obj

llvm/trunk/include/llvm/ObjectYAML/ELFYAML.h

struct Section {
   StringRef Name;
   ELF_SHT Type;
   Optional<ELF_SHF> Flags;
   llvm::yaml::Hex64 Address;
   StringRef Link;
   llvm::yaml::Hex64 AddressAlign;
   Optional<llvm::yaml::Hex64> EntSize;
 
+  // This can be used to override the sh_offset field. It does not place the
+  // section data at the offset specified. Useful for creating invalid objects.
+  Optional<llvm::yaml::Hex64> ShOffset;
+
   Section(SectionKind Kind) : Kind(Kind) {}
   virtual ~Section();
 };
 
 struct DynamicSection : Section {
   std::vector<DynamicEntry> Entries;
   Optional<yaml::BinaryRef> Content;
 

llvm/trunk/lib/ObjectYAML/ELFYAML.cpp

 static void commonSectionMapping(IO &IO, ELFYAML::Section &Section) {
   IO.mapOptional("Name", Section.Name, StringRef());
   IO.mapRequired("Type", Section.Type);
   IO.mapOptional("Flags", Section.Flags);
   IO.mapOptional("Address", Section.Address, Hex64(0));
   IO.mapOptional("Link", Section.Link, StringRef());
   IO.mapOptional("AddressAlign", Section.AddressAlign, Hex64(0));
   IO.mapOptional("EntSize", Section.EntSize);
+
+  // obj2yaml does not dump this field. It is expected to be empty when we are
+  // producing YAML, because yaml2obj sets an appropriate value for sh_offset
+  // automatically when it is not explicitly defined.
+  assert(!IO.outputting() || !Section.ShOffset.hasValue());
+  IO.mapOptional("ShOffset", Section.ShOffset);
 }
 
 static void sectionMapping(IO &IO, ELFYAML::DynamicSection &Section) {
   commonSectionMapping(IO, Section);
   IO.mapOptional("Entries", Section.Entries);
   IO.mapOptional("Content", Section.Content);
 }
 

llvm/trunk/test/tools/yaml2obj/elf-override-shoffset.yaml

+## Check we are able to set custom sh_offset field
+## for different sections.
+
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readelf --sections %t1 | FileCheck %s --check-prefix=CASE1
+
+# CASE1:      Section Headers:
+# CASE1-NEXT:  [Nr] Name           Type     Address Off
+# CASE1-NEXT:  [ 0]                NULL     {{.*}}  000000
+# CASE1-NEXT:  [ 1] .dynsym        DYNSYM   {{.*}}  000001
+# CASE1-NEXT:  [ 2] .symtab        SYMTAB   {{.*}}  000002
+# CASE1-NEXT:  [ 3] .dynamic       DYNAMIC  {{.*}}  000003
+# CASE1-NEXT:  [ 4] .rela          RELA     {{.*}}  000004
+# CASE1-NEXT:  [ 5] .nobits        NOBITS   {{.*}}  000005
+# CASE1-NEXT:  [ 6] .group         GROUP    {{.*}}  000006
+# CASE1-NEXT:  [ 7] .gnu.version   VERSYM   {{.*}}  000007
+# CASE1-NEXT:  [ 8] .gnu.version_r VERNEED  {{.*}}  000008
+# CASE1-NEXT:  [ 9] .gnu.version_d VERDEF   {{.*}}  000009
+# CASE1-NEXT:  [10] .regular       PROGBITS {{.*}}  00000a
+# CASE1-NEXT:  [11] .strtab        STRTAB   {{.*}}  00000b
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .dynsym
+    Type: SHT_DYNSYM
+    ShOffset: 0x000000001
+  - Name: .symtab
+    Type: SHT_SYMTAB
+    ShOffset: 0x000000002
+  - Name: .dynamic
+    Type: SHT_DYNAMIC
+    ShOffset: 0x000000003
+  - Name: .rela
+    Type: SHT_RELA
+    ShOffset: 0x000000004
+  - Name: .nobits
+    Type: SHT_NOBITS
+    ShOffset: 0x000000005
+  - Name: .group
+    Type: SHT_GROUP
+    Info: 0
+    ShOffset: 0x000000006
+    Members:
+  - Name: .gnu.version
+    Type: SHT_GNU_versym
+    Entries: [ ]
+    ShOffset: 0x000000007
+  - Name: .gnu.version_r
+    Type: SHT_GNU_verneed
+    Info: 0x0000000000000001
+    ShOffset: 0x000000008
+    Dependencies:
+  - Name: .gnu.version_d
+    Type: SHT_GNU_verdef
+    Info: 0x0000000000000001
+    ShOffset: 0x000000009
+    Entries:
+  - Name: .regular
+    Type: SHT_PROGBITS
+    ShOffset: 0x00000000A
+  - Name: .strtab
+    Type: SHT_STRTAB
+    ShOffset: 0x00000000B
+
+## Here we check that defining ShOffset does not actually change
+## the offset at which section data is placed and also does
+## not affect file size.
+
+# RUN: yaml2obj --docnum=2 %s -o %t2
+# RUN: yaml2obj --docnum=3 %s -o %t3
+# RUN: od -t x8 -v %t2 > %t.txt
+# RUN: od -t x8 -v %t3 >> %t.txt
+# RUN: FileCheck %s --input-file=%t.txt --check-prefix=CASE2
+
+# CASE2: [[OFFSET:.*]] 8877665544332211
+# CASE2: [[FILESIZE:.*]]{{$}}
+# CASE2: [[OFFSET]] 8877665544332211
+# CASE2: [[FILESIZE]]{{$}}
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+    Content: "1122334455667788"
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+    ShOffset: 0xFFFF0000
+    Content: "1122334455667788"

llvm/trunk/tools/obj2yaml/elf2yaml.cpp

std::error_code ELFDumper<ELFT>::dumpRelocation(const RelT *Rel,
   }
 
   return obj2yaml_error::success;
 }
 
 template <class ELFT>
 std::error_code ELFDumper<ELFT>::dumpCommonSection(const Elf_Shdr *Shdr,
                                                    ELFYAML::Section &S) {
+  // Dump fields. We do not dump the ShOffset field. When not explicitly
+  // set, the value is set by yaml2obj automatically.
   S.Type = Shdr->sh_type;
   if (Shdr->sh_flags)
     S.Flags = static_cast<ELFYAML::ELF_SHF>(Shdr->sh_flags);
   S.Address = Shdr->sh_addr;
   S.AddressAlign = Shdr->sh_addralign;
   if (Shdr->sh_entsize)
     S.EntSize = static_cast<llvm::yaml::Hex64>(Shdr->sh_entsize);
 

llvm/trunk/tools/yaml2obj/yaml2elf.cpp

bool ELFState<ELFT>::initImplicitHeader(ELFState<ELFT> &State,
 
   else if (SecName == ".dynsym")
     State.initSymtabSectionHeader(Header, SymtabType::Dynamic, CBA, YAMLSec);
   else if (SecName == ".dynstr")
     State.initStrtabSectionHeader(Header, SecName, State.DotDynstr, CBA,
                                   YAMLSec);
   else
     return false;
+
+  // Override the sh_offset field if requested.
+  if (YAMLSec && YAMLSec->ShOffset)
+    Header.sh_offset = *YAMLSec->ShOffset;
+
   return true;
 }
 
 static StringRef dropUniqueSuffix(StringRef S) {
   size_t SuffixPos = S.rfind(" [");
   if (SuffixPos == StringRef::npos)
     return S;
   return S.substr(0, SuffixPos);
for (size_t I = 1; I < Doc.Sections.size() + ImplicitSections.size() + 1; ++I) {
     } else if (auto S = dyn_cast<ELFYAML::VerneedSection>(Sec)) {
       if (!writeSectionContent(SHeader, *S, CBA))
         return false;
     } else if (auto S = dyn_cast<ELFYAML::VerdefSection>(Sec)) {
       if (!writeSectionContent(SHeader, *S, CBA))
         return false;
     } else
       llvm_unreachable("Unknown section type");
+
+    // Override the sh_offset field if requested.
+    if (Sec && Sec->ShOffset)
+      SHeader.sh_offset = *Sec->ShOffset;
   }
 
   return true;
 }
 
 static size_t findFirstNonGlobal(ArrayRef<ELFYAML::Symbol> Symbols) {
   for (size_t I = 0; I < Symbols.size(); ++I)
     if (Symbols[I].Binding.value != ELF::STB_LOCAL)