[yaml2obj] - Automatically assign sh_addr for allocatable sections.

[grimar]

Summary

I've noticed that it is not convenient to create YAMLs from
binaries (using obj2yaml) that have to be test cases for obj2yaml
later (after applying yaml2obj).

The problem, for example is that obj2yaml emits "DynamicSymbols:"
key instead of .dynsym. It also does not create .dynstr.
And when a YAML document without explicitly defined .dynsym/.dynstr
is given to yaml2obj, we have issues:

1. These sections are placed after non-allocatable sections (I've fixed it in D74756).
2. They have VA == 0. User needs create descriptions for such sections explicitly manually to set a VA.

This patch addresses (2). I suggest to let yaml2obj assign virtual addresses by itself.
It makes an output binary to be much closer to "normal" ELF.
(It is still possible to use "Address: 0x0" for a section to get the original behavior
if it is needed)

[MaskRay]

I think this is the correct approach. It will definitely simplify tests.

Nit: in the description, use sh_addr instead of VA because sh_addr may be more familiar to readers.

llvm/lib/ObjectYAML/ELFEmitter.cpp

222

Section *

397

Add a test that ShSize (overriding sh_size, usually used for abnormal values) does not affect LocationCounter.

527

Delete. I think non-SHF_ALLOC sections follow the same rule.

531

getValueOr

llvm/test/tools/yaml2obj/ELF/section-address-assign.yaml

31

0x1000 is fine but probably not the best.

If .text.any.addr is included by a PT_LOAD, one will expect sh_offset%p_align = sh_addr%p_align (a PT_LOAD's p_align is usually 0x1000 (maxpagesize) on x86).

One approach is to specify sh_addralign and set sh_addr to sh_addralign plus a multiple of p_align.

Address: 0x1200
AddressAlign: 0x200

[MaskRay]

llvm/test/tools/yaml2obj/ELF/section-address-assign.yaml

54

Add a ShSize test somewhere.

64

Add a non-SHF_ALLOC section (e.g. .rela.debug_info) to test this rule applies on non-SHF_ALLOC sections.

[grimar]

• Addressed review comments.

llvm/lib/ObjectYAML/ELFEmitter.cpp

527

Not sure I follow. We should not assign sh_addr for non-allocatable sections. Isn't?
In LLD we just drop counter to 0:

void LinkerScript::assignOffsets(OutputSection *sec) {
  if (!(sec->flags & SHF_ALLOC))
     dot = 0;

GNU linkers do not set addresses to sh_addr for non-allocatable sections too.
Why should yaml2obj do different thing?

531

getValueOr, which is a method of Optional<>? There is no optional here. I am testing sh_addralign for 0 because
alignTo expects it to be !=0:

inline uint64_t alignTo(uint64_t Value, uint64_t Align, uint64_t Skew = 0) {
  assert(Align != 0u && "Align can't be 0.");
...

llvm/test/tools/yaml2obj/ELF/section-address-assign.yaml

31

I do not think it is a problem here. yaml2obj do just what it do: produce an output from a given description.
This test just documents the behavior of assigning addresses. It does not have intention to create an
ELF that has no issues.

And so it probably does not make much sence to work hard and put 0x1200 and AddressAlign here.
At least because it works only for .text*. See, the offset of .data* is broken:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00     0   0  0
  [ 1] .text.any.addr    PROGBITS        0000000000001200 000200 000003 00   A 0   0 512
  [ 2] .text.shsize      PROGBITS        0000000000001203 000203 001234 00   A 0   0  0
  [ 3] .text.align       PROGBITS        0000000000001300 000300 000004 00   A 0   0 256
  [ 4] .data.any.addr    PROGBITS        0000000000002000 000304 000001 00   A 0   0  0
  [ 5] .data.after.fill  PROGBITS        0000000000002101 000405 000001 00   A 0   0  0
  [ 6] .data.return.back PROGBITS        0000000000001500 000406 000001 00   A 0   0  0
  [ 7] .data.return.back.foo PROGBITS    0000000000001501 000407 000000 00   A 0   0  0
  [ 8] .dynsym           DYNSYM          0000000000001508 000408 000018 18   A 9   1  8
  [ 9] .dynstr           STRTAB          0000000000001520 000420 000001 00   A 0   0  1
  [10] .strtab           STRTAB          0000000000000000 000421 000001 00 0   0  1
  [11] .shstrtab         STRTAB          0000000000000000 000422 000093 00 0   0  1

Yes, it is possible to adjust other sections too, add a few properties to make them follow the rule, but that requires adding
additional fields, and I am not sure I see how it is useful in the context of this test case.
I see why it can be bad though :) It would make the test case larger than it can be.

64

We have .strtab and .shstrtab added implicitly. They are non-SHF_ALLOC.
I think I just do not understand what exactly you want me to test for them?

[grimar]

Now I am going to fix all test cases that are failing and remove the [WIP] status after that update.

[MaskRay]

The code change looks good to me now.

llvm/test/tools/yaml2obj/ELF/section-address-assign.yaml

64

.shstr and .shstrtab are non-SHF_ALLOC. I think it is fine not adding another non-SHF_ALLOC section.

[grimar]

• Fixed test cases.
• Disabled auto VA assign for sections in relocatable objects.

[MaskRay]

llvm/lib/ObjectYAML/ELFEmitter.cpp

529

The two conditions can be combined.

sh_addr represents the address in the memory image of a process. Sections in a relocatable object file or non-allocatable sections do not need sh_addr assignment.

llvm/test/tools/yaml2obj/ELF/section-address-assign.yaml

2

Test that yaml2obj automatically assigns sh_addr to allocatable sections for ET_EXEC/ET_DYN files.

(ET_CORE is possible but they don't usually have the section header table.)

[grimar]

• Addressed review comments.

[jhenderson]

Sorry, I didn't get a chance to look at this earlier. It's been a pretty hectic few days. Two comment-related nits from me.

Also, not worth fixing them now, but in some of those tests, we probably didn't need to check the actual address value at all, so they could have been replaced with {{.*}} or similar.

llvm/lib/ObjectYAML/ELFEmitter.cpp

707–709

Nit: this comment looks stale now.

llvm/test/tools/yaml2obj/ELF/section-address-assign.yaml

87

I think you mean "go back" not "return back".

[grimar]

In D74764#1888953, @jhenderson wrote:

Two comment-related nits from me.

Thanks, I've addressed them in https://reviews.llvm.org/rG157b3d505f6

Also, not worth fixing them now, but in some of those tests, we probably didn't need to check the actual address value at all, so they could have been replaced with {{.*}} or similar.

In some of them the check for address seems to be useful thing. Like in llvm-readobj tests where we now checking that we dump addresses properly.

llvm/include/llvm/ObjectYAML/ELFYAML.h

struct Chunk {
 
   Chunk(ChunkKind K) : Kind(K) {}
   virtual ~Chunk();
 };
 
 struct Section : public Chunk {
   ELF_SHT Type;
   Optional<ELF_SHF> Flags;
-  llvm::yaml::Hex64 Address;
+  Optional<llvm::yaml::Hex64> Address;
   StringRef Link;
   llvm::yaml::Hex64 AddressAlign;
   Optional<llvm::yaml::Hex64> EntSize;
 
   // Usually sections are not created implicitly, but loaded from YAML.
   // When they are, this flag is used to signal about that.
   bool IsImplicit;
 

llvm/lib/ObjectYAML/ELFEmitter.cpp

template <class ELFT> class ELFState {
   /// The future ".dynstr" section.
   StringTableBuilder DotDynstr{StringTableBuilder::ELF};
 
   NameToIdxMap SN2I;
   NameToIdxMap SymN2I;
   NameToIdxMap DynSymN2I;
   ELFYAML::Object &Doc;
 
+  uint64_t LocationCounter = 0;
   bool HasError = false;
   yaml::ErrorHandler ErrHandler;
   void reportError(const Twine &Msg);
 
   std::vector<Elf_Sym> toELFSymbols(ArrayRef<ELFYAML::Symbol> Symbols,
                                     const StringTableBuilder &Strtab);
   unsigned toSectionIndex(StringRef S, StringRef LocSec, StringRef LocSym = "");
   unsigned toSymbolIndex(StringRef S, StringRef LocSec, bool IsDynamic);
template <class ELFT> class ELFState {
   void writeSectionContent(Elf_Shdr &SHeader,
                            const ELFYAML::CallGraphProfileSection &Section,
                            ContiguousBlobAccumulator &CBA);
 
   void writeFill(ELFYAML::Fill &Fill, ContiguousBlobAccumulator &CBA);
 
   ELFState(ELFYAML::Object &D, yaml::ErrorHandler EH);
 
+  void assignSectionAddress(Elf_Shdr &SHeader, ELFYAML::Section* YAMLSec);
+
 public:
   static bool writeELF(raw_ostream &OS, ELFYAML::Object &Doc,
                        yaml::ErrorHandler EH);
 };
 } // end anonymous namespace
 
 template <class T> static size_t arrayDataSize(ArrayRef<T> A) {
   return A.size() * sizeof(T);
else if (SecName == ".shstrtab")
     initStrtabSectionHeader(Header, SecName, DotShStrtab, CBA, YAMLSec);
   else if (SecName == ".dynsym")
     initSymtabSectionHeader(Header, SymtabType::Dynamic, CBA, YAMLSec);
   else if (SecName == ".dynstr")
     initStrtabSectionHeader(Header, SecName, DotDynstr, CBA, YAMLSec);
   else
     return false;
 
+  LocationCounter += Header.sh_size;
+
   // Override section fields if requested.
   overrideFields<ELFT>(YAMLSec, Header);
   return true;
 }
 
 StringRef llvm::ELFYAML::dropUniqueSuffix(StringRef S) {
   size_t SuffixPos = S.rfind(" [");
   if (SuffixPos == StringRef::npos)
     return S;
   return S.substr(0, SuffixPos);
 }
 
 template <class ELFT>
 void ELFState<ELFT>::initSectionHeaders(std::vector<Elf_Shdr> &SHeaders,
                                         ContiguousBlobAccumulator &CBA) {
   // Ensure SHN_UNDEF entry is present. An all-zero section header is a
   // valid SHN_UNDEF entry since SHT_NULL == 0.
   SHeaders.resize(Doc.getSections().size());
 
   size_t SecNdx = -1;
   for (const std::unique_ptr<ELFYAML::Chunk> &D : Doc.Chunks) {
     if (auto S = dyn_cast<ELFYAML::Fill>(D.get())) {
       writeFill(*S, CBA);
+      LocationCounter += S->Size;
       continue;
     }
 
     ++SecNdx;
     ELFYAML::Section *Sec = cast<ELFYAML::Section>(D.get());
     if (SecNdx == 0 && Sec->IsImplicit)
       continue;
 
for (const std::unique_ptr<ELFYAML::Chunk> &D : Doc.Chunks) {
     assert(Sec && "It can't be null unless it is an implicit section. But all "
                   "implicit sections should already have been handled above.");
 
     SHeader.sh_name =
         DotShStrtab.getOffset(ELFYAML::dropUniqueSuffix(Sec->Name));
     SHeader.sh_type = Sec->Type;
     if (Sec->Flags)
       SHeader.sh_flags = *Sec->Flags;
-    SHeader.sh_addr = Sec->Address;
     SHeader.sh_addralign = Sec->AddressAlign;
 
+    assignSectionAddress(SHeader, Sec);
+
     if (!Sec->Link.empty())
       SHeader.sh_link = toSectionIndex(Sec->Link, Sec->Name);
 
     if (SecNdx == 0) {
       if (auto RawSec = dyn_cast<ELFYAML::RawContentSection>(Sec)) {
         // We do not write any content for special SHN_UNDEF section.
         if (RawSec->Size)
           SHeader.sh_size = *RawSec->Size;
for (const std::unique_ptr<ELFYAML::Chunk> &D : Doc.Chunks) {
     } else if (auto S = dyn_cast<ELFYAML::DependentLibrariesSection>(Sec)) {
       writeSectionContent(SHeader, *S, CBA);
     } else if (auto S = dyn_cast<ELFYAML::CallGraphProfileSection>(Sec)) {
       writeSectionContent(SHeader, *S, CBA);
     } else {
       llvm_unreachable("Unknown section type");
     }
 
+    LocationCounter += SHeader.sh_size;
+
     // Override section fields if requested.
     overrideFields<ELFT>(Sec, SHeader);
   }
 }
 
+template <class ELFT>
+void ELFState<ELFT>::assignSectionAddress(Elf_Shdr &SHeader,
+                                          ELFYAML::Section *YAMLSec) {
+  if (YAMLSec && YAMLSec->Address) {
+    SHeader.sh_addr = *YAMLSec->Address;
+    LocationCounter = *YAMLSec->Address;
+    return;
+  }
+
+  if (!(SHeader.sh_flags & ELF::SHF_ALLOC))
+    return;
+
+  LocationCounter =
+      alignTo(LocationCounter, SHeader.sh_addralign ? SHeader.sh_addralign : 1);
+  SHeader.sh_addr = LocationCounter;
+}
+
 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)
       return I;
   return Symbols.size();
 }
 
 static uint64_t writeContent(raw_ostream &OS,
void ELFState<ELFT>::initSymtabSectionHeader(Elf_Shdr &SHeader,
   // If the symbol table section is explicitly described in the YAML
   // then we should set the fields requested.
   SHeader.sh_info = (RawSec && RawSec->Info) ? (unsigned)(*RawSec->Info)
                                              : findFirstNonGlobal(Symbols) + 1;
   SHeader.sh_entsize = (YAMLSec && YAMLSec->EntSize)
                            ? (uint64_t)(*YAMLSec->EntSize)
                            : sizeof(Elf_Sym);
   SHeader.sh_addralign = YAMLSec ? (uint64_t)YAMLSec->AddressAlign : 8;
-  SHeader.sh_addr = YAMLSec ? (uint64_t)YAMLSec->Address : 0;
+
+  assignSectionAddress(SHeader, YAMLSec);
 
   auto &OS = CBA.getOSAndAlignedOffset(SHeader.sh_offset, SHeader.sh_addralign);
   if (RawSec && (RawSec->Content || RawSec->Size)) {
     assert(Symbols.empty());
     SHeader.sh_size = writeContent(OS, RawSec->Content, RawSec->Size);
     return;
   }
 
void ELFState<ELFT>::initStrtabSectionHeader(Elf_Shdr &SHeader, StringRef Name,
   if (RawSec && RawSec->Info)
     SHeader.sh_info = *RawSec->Info;
 
   if (YAMLSec && YAMLSec->Flags)
     SHeader.sh_flags = *YAMLSec->Flags;
   else if (Name == ".dynstr")
     SHeader.sh_flags = ELF::SHF_ALLOC;
 
   // If the section is explicitly described in the YAML
   // then we want to use its section address.
-  if (YAMLSec)
-    SHeader.sh_addr = YAMLSec->Address;
+  assignSectionAddress(SHeader, YAMLSec);
 }
 
 template <class ELFT> void ELFState<ELFT>::reportError(const Twine &Msg) {
   ErrHandler(Msg);
   HasError = true;
 }
 
 template <class ELFT>

llvm/lib/ObjectYAML/ELFYAML.cpp

if (Symbol.Index && Symbol.Section.data())
     return "Index and Section cannot both be specified for Symbol";
   return StringRef();
 }
 
 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("Address", Section.Address);
   IO.mapOptional("Link", Section.Link, StringRef());
   IO.mapOptional("AddressAlign", Section.AddressAlign, Hex64(0));
   IO.mapOptional("EntSize", Section.EntSize);
 
   // 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() ||

llvm/test/tools/yaml2obj/ELF/section-address-assign.yaml

+## This test shows how yaml2obj automatically assigns virtual addresses for sections. 
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readelf --sections %t | FileCheck %s
+
+# CHECK:      Section Headers:
+# CHECK-NEXT:   [Nr] Name                  Type     Address          Off    Size   ES Flg Lk Inf Al
+# CHECK-NEXT:   [ 0]                       NULL     0000000000000000 000000 000000 00     0   0  0
+# CHECK-NEXT:   [ 1] .text.any.addr        PROGBITS 0000000000001000 000040 000003 00   A 0   0  0
+# CHECK-NEXT:   [ 2] .text.align           PROGBITS 0000000000001100 000100 000004 00   A 0   0 256
+# CHECK-NEXT:   [ 3] .data.any.addr        PROGBITS 0000000000002000 000104 000001 00   A 0   0  0
+# CHECK-NEXT:   [ 4] .data.after.fill      PROGBITS 0000000000002101 000205 000001 00   A 0   0  0
+# CHECK-NEXT:   [ 5] .data.return.back     PROGBITS 0000000000001500 000206 000001 00   A 0   0  0
+# CHECK-NEXT:   [ 6] .data.return.back.foo PROGBITS 0000000000001501 000207 000000 00   A 0   0  0
+# CHECK-NEXT:   [ 7] .dynsym               DYNSYM   0000000000001508 000208 000018 18   A 8   1  8
+# CHECK-NEXT:   [ 8] .dynstr               STRTAB   0000000000001520 000220 000001 00   A 0   0  1
+# CHECK-NEXT:   [ 9] .strtab               STRTAB   0000000000000000 000221 000001 00     0   0  1
+# CHECK-NEXT:   [10] .shstrtab             STRTAB   0000000000000000 000222 000086 00     0   0  1
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+## Show we can place a section at any address.
+  - Name:    .text.any.addr
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC ]
+    Address: 0x1000
+    Size:    0x3
+## Show we respect an address align when automatically
+## assign virtual addresses.
+  - Name:         .text.align
+    Type:         SHT_PROGBITS
+    Flags:        [ SHF_ALLOC ]
+    AddressAlign: 0x100
+    Size:         0x4
+## We can set another address for a subsequent section.
+  - Name:    .data.any.addr
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC ]
+    Address: 0x2000
+    Size:    0x1
+## Show that Fill occupies VA space.
+  - Type:    Fill
+    Pattern: "AABB"
+    Size:    0x100
+  - Name:  .data.after.fill
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_ALLOC ]
+    Size:  0x1
+## Show we can return back in the address space and
+## continue placing sections. The order of sections in the
+## section header table will match the order in the YAML description.
+  - Name:    .data.return.back
+    Address: 0x1500
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC ]
+    Size:    0x1
+  - Name:    .data.return.back.foo
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC ]
+## Used to trigger creation of .dynsym and .dynstr.
+DynamicSymbols: []

llvm/tools/obj2yaml/elf2yaml.cpp

 template <class ELFT>
 Error 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.Address = static_cast<uint64_t>(Shdr->sh_addr);
   S.AddressAlign = Shdr->sh_addralign;
   if (Shdr->sh_entsize)
     S.EntSize = static_cast<llvm::yaml::Hex64>(Shdr->sh_entsize);
 
   auto NameOrErr = getUniquedSectionName(Shdr);
   if (!NameOrErr)
     return NameOrErr.takeError();
   S.Name = NameOrErr.get();