[ifs] Allow llvm-ifs to generate text stub from elf stub

[haowei]

Summary

ELF stubs generated from llvm-ifs lacks program headers, which prevents llvm-ifs from parsing them properly as program headers are required by llvm's own ELF libraries. This patch adds a few workaround bypass this limitation.

llvm::Object::ElfFile::toMappedAddr() function requires valid program header, which was used by llvm-ifs's elf parser to determine the pointers to the strtab and dynsym sections. Instead of adding program header sections to the ELF stub output, this change reads .dynsym section from section headers and use sh_link and sh_offset to calculate the pointers to the required sections. If these step fails, it then fallbacks to the original method of using program headers. If both methods failed, An error will be thrown.

[mcgrathr]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

592

This value cannot be safely used at all. The DT_STRTAB address is not something that can be presumed to be an offset into the file, even if that's often true.

The behavior I'd expect here is to first look for section headers. If there are section headers at all, then purely look for the SHT_DYNSYM section and follow its sh_link to its SHT_STRTAB section and use its sh_offset and sh_size. If there are section headers at all, but no SHT_DYNSYM section or its sh_link doesn't point to an SHT_STRTAB section, then you don't need to handle this input file. It's invalid or empty.

Only if there are no section headers at all should you fall back to decoding based on phdrs. This should be the same for finding the SHT_DYNAMIC section in the first place, only falling back to PT_DYNAMIC if there are no section headers at all.

Also note that for DT_NEEDED et al, strings pointed to by SHT_DYNAMIC entries, technically it's the sh_link of the SHT_DYNAMIC that tells you what string table section to use. In practice, it's always the same one as the one that SHT_DYNSYM's sh_link points to, but by the spec they can be separate when dealing with sections.

631

Same issues here.

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

592

I have corrected the pointers to the .dynstr and .dynsym according to your suggestions.

I will submit another patch to correct the DT_NEEDED as it is out of the scope of this patch. Thanks for pointing that out.

[mcgrathr]

LGTM modulo the fallback behaviors as mentioned below. But I'll leave the approval to those who have done the review for this specific codebase more generally, since I'm not sure on all the points of LLVM style or what library features are available or best to use.

[mcgrathr]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

588

I would actually fall back here only if there are no section headers at all. If there are any section headers at all but no SHT_DYNSYM section, then don't look further. i.e. return failure after the for loop above inside the if.

594

This is inconsistent with the Shdrs case above wrt whether or not to use bool coercion for a zero check.
I personally like avoiding the implicit coercion and spelling it out like you did here.
But I think it's most important to be consistent in style for all the integer cases (using implicit bool coercion for pointer nullptr checks is canonical style in C++, but for integers it's less clear).

629

Same here wrt the fallback logic.

632

What's the purpose of fetching and checking PHdrs here when you just use toMappedAddr (which I presume only works if the phdrs were present to tell it what to do)?

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

588

If it fail when .dynsym is absent, test https://github.com/llvm/llvm-project/blob/ebd8eee62a431a6744c3f187fcda58e5dea08499/llvm/test/tools/llvm-ifs/binary-read-add-soname.test will fail (there are 7 similar tests) as there is no .dynsym at all, .dynstr (though empty) is pointed by .dynamic . I believe either fallback strategy won't cause any issues in the field.

If an ELF has no symbols (which is unlikely), should it have no .dynsym section or should it have an empty .dynsym?

594

Thanks for pointing this out. I will delete this check since toMappedAddr will fail with an empty program headers. I can re-add the check if you think it is necessary to add a more detailed error message when the program header is empty.

632

It was a check before toMappedAddr since this function will fail if program headers are empty. I will remove the check.

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

588

I uploaded a patch using your suggested fallback logic and modified affected tests (and deleted one as it not achievable).

[mcgrathr]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

588

No SHT_DYNSYM section could ever be empty, because index 0 is reserved and must contain a stub entry.

I've never seen a linker that produces a .dynamic at all without having a .dynsym. clang -nostdlib -shared -o /tmp/foo.so -xassembler /dev/null with a toolchain that uses current LLD by default produces a .dynsym that has nothing but the null entry. With some current or past GNU linkers, that degenerate case would still produce either some local STT_SECTION symbols in .dynsym (which really never had a reason to be there), or always export some symbols like _end or _edata in .dynsym so it was never empty of symbols even when it probably should have been.

So I think basically, if there are section headers at all and this is a real-world ET_DYN file of any plausible normal sort of provenance that development tools were ever really expected to handle, then finding no SHT_DYNSYM at all is more likely to indicate a corrupted file or tool bug than a degenerate ELF file that's actually harmless. Likewise, if you're looking at PT_DYNAMIC data via phdrs instead, then once you've found the dynamic section then if it doesn't include all of DT_{SYMTAB,SYMENT,STRTAB,STRSZ} then it's more likely to be a corrupt image than a valid degenerate ELF file with no dynamic symbol table. Even a static PIE that has no dynamic relocs that require referring to a symbol table at all (only simple fixups) will in practice still have a symbol table with the null entry at index 0 and an empty string table but with an address and headers as if it had a length other than zero.

594

I think whatever form of "the memory access didn't work" error it produces is fine without being any more specific about why it failed for this case than it is for any other case.

[jhenderson]

I've not really dug into this patch at this point, but one high-level thing I wanted to highlight: it looks like you're doing a lot of reading of the object file to interpret the contents, in ways that tools like llvm-readelf and llvm-objdump already do. Do you actually need this much additional code as a result? It might need some refactoring, but it seems like we're otherwise reinventing the wheel...!

llvm/lib/InterfaceStub/ELFObjHandler.cpp

229

This feels like it should be its own independent fix, with corresponding testing etc.

588

FWIW, we have a proprietary linker that sticks dynamic data like the dynsym and dynstr inside a combined section, pointed to by dynamic tags contained in a (normal) dynamic section. As such, in our case, we would have a .dynamic, but no .dynstr or .dynsym (in the section header table). We don't use this tool however, and I know our case is unusual, so don't feel a need to handle it necessarily - just highlighting that it is a possible case (just not with, to my knowledge, mainstream linkers).

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

229

I splitted it into https://reviews.llvm.org/D117058. Please take a look.

588

I am slightly lean a bit more with the fallback logic in diff3 (if .dynsym is not found, fallback to use program header and pointers from .dynamic) instead of exit early when .dynsym is not found. I bet there will be other elf tools in the wild that may generate ELF files with not common sections arrangements and it may help to avoid hassles in the future. What do you think?

[haowei]

In D116769#3230703, @jhenderson wrote:

I've not really dug into this patch at this point, but one high-level thing I wanted to highlight: it looks like you're doing a lot of reading of the object file to interpret the contents, in ways that tools like llvm-readelf and llvm-objdump already do. Do you actually need this much additional code as a result? It might need some refactoring, but it seems like we're otherwise reinventing the wheel...!

I agree it feels like re-inventing the wheel. I spent sometime reading llvm-objdump and llvm-readobj 's code and actually they both have their own dedicated ELF dumper implementation and they are not interchangeable. While theoretically it is possible to do refractor so all 3 tools can use the same ELF parsing implementation, but it looks like a large amount of work. In both llvm-objdump and llvm-readobj's case, the ELF dumper implementation did not provide any interface to access sections or symbol tables, instead, both tool just print the section data into IO streams, making them difficult to reuse. In llvm-ifs's case, most of ELF parsing is for extracting symbol tables from the ELF file, other sections like .rela are completely ignored. The usages and requirements of all 3 tools are not entirely overlapped.

[mcgrathr]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

588

SGTM. The suggestion to bail early was based on the idea that it might be a sign of corrupt input rather than anything else in practice. Even just knowing that there exists any linker that produces output that doesn't match the assumptions I stated is reason enough to be more accepting of any input we can find a way to work with.

[haowei]

Change the fallback logics to Diff3. Also adds parents revision to make presubmit check happy.

[jhenderson]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

575–577

LLVM style says no braces for simple if/loop statements. Also applies below.

You might want some blank lines to break up the stages of this function, especially after deleting the unnecessary braces.

581

582

The fact that the getDynSymtabSize function exists makes me think we should have a function getDynSymtabSection function in the ElfFile interface. Possibly this getDynStr function should also be moved there.

595

I'm not sure if the doxygen style comments are really needed on static functions, but I'm not necessarily opposed to them.

599

Similar note to above, maybe this should be part of the ElfFile interface?

605

Same as above re. braces, here and below.

608

Noting that most of this function is almost identical to above, so probably you want to have a function that finds a dynsym section and returns it, so that these two functions can work on them as appropriate.

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

582

I tried this idea in https://gist.github.com/zeroomega/3d19c04cdc489d99f5ff0d0974b61772 but I am kind of regretting it. It makes the fallback logic a bit messy, partially due to how llvm::Error is constructed. Also I still need to fetch ElfFile.sections() again when extracting the StringRef so it didn't actually make this function more tidy.

599

See the explanation above.

605

I think I still need braces for L549 to L557 after reading the examples in https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

[jhenderson]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

581

You missed the additional "are".

608

How about having a function that takes a lambda "OnFound", to share the majority of the logic between these two functions (that may or may not then be sensible to add to the ElfFile interface). Approximate untested implementation and usage:

template <class ELFT>
Expected<const uint8_t*> getSectionFromDynsym(const ElfFile<ELFT> &ElfFile, ELFT::Elf_Addr DynAddr, StringRef Name, function_ref<Expected<const uint8_t *>(const ELFT::Shdr &, const ELFT::ShdrRange &) OnFound) {
    using Elf_Shdr_Range = typename ELFT::ShdrRange;
  using Elf_Shdr = typename ELFT::Shdr;

  Expected<Elf_Shdr_Range> Shdrs = ElfFile.sections();
  if (!Shdrs)
    return Shdrs.takeError();

  if (Shdrs.get().size()) {
    for (const Elf_Shdr &Sec : *Shdrs) {
      if (Sec.sh_type == ELF::SHT_DYNSYM) {
        // If multiple .dynsym are present, use the first one.
        // This behavior aligns with llvm::object::ELFFile::getDynSymtabSize().
        return OnFound(Sec, *Shdrs);
      }
    }
  }  // Fallback to use offsets from .dynamic as .dynsym was not found.
  Expected<const uint8_t *> PtrOrErr =
      ElfFile.toMappedAddr(DynAddr);
  if (!PtrOrErr)
    return appendToError(PtrOrErr.takeError(),
                         "when locating " + Name + " section contents");
  return *PtrOrErr;
}

template <class ELFT>
static Expected<const uint8_t *> getDynSymPtr(const ELFFile<ELFT> &ElfFile,
                                              const DynamicEntries &DynEnt) {
  return getSectionFromDynsym(ElfFile, DynEnt.DynSymAddr, "dynamic symbol table", [&ElfFile](const ELFT::Elf_Shdr &Sec, const ELFT::ShdrRange &Shdrs){ return ElfFile.base() + Sec.sh_offset(); });
}

template <class ELFT>
static Expected<StringRef> getDynStr(const ELFFile<ELFT> &ElfFile,
                                              const DynamicEntries &DynEnt) {
  Expected<StringRef> ResultOrErr = getSectionFromDynsym(ElfFile, DynEnt.DynStrAddr, "dynamic string table", [&ElfFile](const ELFT::Elf_Shdr &Sec, const ELFT::ShdrRange &Shdrs){ return ElfFile.getStringTableForSymtab(Sec, *Shdrs); });
  if (!ResultOrErr)
    return ResultOrErr;
  return StringRef(reinterpret_cast<const char *>(*ResultOrErr), DynEnt.StrSize);
}

I've also made some more idiomatic changes to the names and usage of the Expected at the end, which you should probably adopt anyway (specifically using "*OrErr" for the variable name, and using operator* to get the real value). Also, as a related aside, I'd avoid using explicitly ".dynsym" and ".dynstr" as names if you're looking up by type, as there is a chance these sections aren't named that. My example code should illustrate it better, hopefully.

611

Ditto (additional "are" needed).

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

608

Thanks for providing this code snippet, I modified it and finished a version that passed all the test: https://gist.github.com/zeroomega/9552161efca45ad8af17a3f312582753

Unfortunately I don't think it can achieve the goal we want. IMO the lambda solution is a bit less readable. And it is about 5lines longer if you strip the comments. I think the cause is that getDynStr and getDynSym has 2 different type of return values. The first one returns a StringRef while the other one returns a raw pointer. The API getStringTableForSymtab used in getDynStr has the same return type. If both function used a common helper function, the runtion will need to return raw pointer, and that is where all the conversion needed. If you look at L50, I have to convert StringRef back to raw pointer to accommodate that. And I have to check the Expect<T> before the conversion, otherwise I am getting Expected<T> must be checked before access or destruction. error.

[jhenderson]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

608

My main goal is removing the near-identical code between these two functions, as duplicated code will lead to more maintenance problems down the line. I have two ideas to resolve this:

1. We could solve this issue by changing the signature of getDynSymPtr to return an Expected<StringRef>. A cast could be added to convert within the lambda, if needed, but it doesn't make it significantly more complex. The call site would need a small tweak too, but it could happen after the Expected has been checked (a quick look shows there's already a cast, so it just would require addition of a .data() call somewhere when doing the existing cast).

2. Another idea I had would be to somehow template the return type. I believe this isn't an option using plain old functions, but you could wrap it in a templated class, I believe, and make the function a member of that class.

I'm not sure which would look cleaner overall, but both should work, and resolve the duplicated code.

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

608

For suggestion 1. That is a hard no for me. DynSym data are binary I don't think it is wise to fill it into a StringRef which is suppose to just host references to strings. For suggestion 2. I would like to avoid use template class type for return value. It does not help the readability and makes the problem complicated. Instead, I implemented a solution in Diff 9, in which I reuse the code as best as I can. Please take a look.

I am still lean to Diff 8 since it is more straight forward and easier to understand. But if you are insist on no code should be repeated, I am fine with the solution in Diff 9.

[haowei]

If there is no further objection or comments, can I get a LGTM to land this change please?

[jhenderson]

In D116769#3270596, @haowei wrote:

If there is no further objection or comments, can I get a LGTM to land this change please?

Sorry, I've been off for two weeks, and only now getting back to these things.

llvm/lib/InterfaceStub/ELFObjHandler.cpp

576–577

This should be an assert.

583

No need for this if: the loop will do nothing if Shdrs is empty.

594–596

No need for else after return

604

I'd probably refer to them as "dynamic string table" and "dynamic symbol table", rather than by section names, since they could be called other things.

608

That is a hard no for me. DynSym data are binary I don't think it is wise to fill it into a StringRef which is suppose to just host references to strings.

Fine. Use a return type of ArrayRef<char>, and use data() and size() to construct the value then... A StringRef is a reference to an array of chars, so it's really the same... I'm pretty sure StringRef is often used in that context, even for binary data.

The current implementation in this patch isn't terrible, but it's not particularly nice either: the Type argument turns the function into a bi-modal function, so this function is being used to do two different things, which isn't really good software design. I think we can do better.

Latest alternate idea: have a class (perhaps simply called "DynSym" or similar), which during initialisation looks up the dynsym section header and stores it in a pointer, leaving it as null if not found. Then make the getDynSym and getDynStr functions member functions, which simply use the precalculated info to return the data as appropriate. This way, the shared data (the dynsym section header) is only looked up once. The two separate methods do only the behaviour that is specific to the relevant table you're looking up. Untested implementation:

class DynSym {
  using Elf_Shdr_Range = typename ELFT::ShdrRange;
  using Elf_Shdr = typename ELFT::Shdr;

public:
  static Expected<DynSym> create(const ELFFile<ELFT> &ElfFile, const DynamicEntries &DynEnt) {
      Expected<Elf_Shdr_Range> Shdrs = ElfFile.sections();
      if (!Shdrs)
        return Shdrs.takeError();
      return DynSym(ElfFile, DynEnt, *Shdrs);
  }

  Expected<const uint8_t*> getDynSym() {
    if(DynSymHdr)
      return ElfFile.base() + DynSymHdr->sh_offset;
    return getDynamicData(DynEnt.DynSymAddr, "dynamic symbol table");
  }

  // StringRef or uint8_t* return according to taste as to where the casts go.
  Expected<StringRef> getDynStr() {
    if(DynSymHdr)
      return ElfFile.getStringTableForSymtab(DynSymHdr, Shdrs);
    Expected<const uint8_t *> DataOrErr = getDynamicData(DynEnt.DynSymAddr, "dynamic string table");
    if (!DataOrErr)
      return DataOrErr.takeError();
    return StringRef(*DataOrErr, DynEnt.StrSize);
  }

private:
  DynSym(const ELFFile<ELFT> &ElfFile, const DynamicEntries &DynEnt, Elf_Shdr_Range Shdrs)
    : ElfFile(ElfFile), DynEnt(DynEnt), Shdrs(Shdrs), DynSymHdr(findDynSymHdr(Shdrs)) {}

  Elf_Shdr *findDynSymHdr() {
    // Probably use find_if?
    for (const Elf_Shdr &Sec : Shdrs)
      if (Sec.sh_type == SHT_DYNSYM)
        return &Sec;
    return nullptr;
  }

  Expected<const uint8_t *> getDynamicData(uint64_t Address, StringRef Name) {
      Expected<const uint8_t *> SecPtr = ElfFile.toMappedAddr(EntAddr);
      if (!SecPtr)
        return appendToError(SecPtr.takeError(), "when locating " + Name + " section contents");
      return *SecPtr;
  }

  const ELFFile<ELFT> &ElfFile;
  const DynamicEntries &DynEnt;
  Elf_Shdr_Range Shdrs;
  Elf_Shdr *DynSymHdr;
};

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

608

Thanks for the suggestion. I think this is better than the solutions earlier and I have rewrite this patch using the class you suggested. I think overall it looks cleaner and easier to understand.

[jhenderson]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

374

Sorry, this comment was intended purely for illustration of my example! You don't need it in the real code.

402–404

StringRef + string literals converts to llvm::Twine. Prefer that for concatenation and then use .str() to convert back to a std::string afterwards, as it's more efficient.

588

I think you can just do the inline edit.

592

I'm pretty sure this is more idiomatic.

628

llvm/test/tools/llvm-ifs/ifs-elf-conversion.test

3–5

As the test is the sequence of the two commands, rather than the two commands being independent, I think the blank line between them should be removed.

[jhenderson]

Looks reasonable to me. You probably want to give @mcgrathr a chance to confirm too.

[mcgrathr]

lgtm except for possible susceptibility to invalid DT_STRSZ values.
Perhaps there should be a test case for a corrupted PT_DYNAMIC region.

llvm/lib/InterfaceStub/ELFObjHandler.cpp

382

Is there anything that validates that this size is within the bounds of the memory returned by toMappedAddr?

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

382

There is no validating in this implementation and I think that is an issue. It would need a rewrite or reimplement of toMappedAddr to validate if the size is within the bounds of the same PT_LOAD segment returned by toMappedAddr. But if we just want validate if the size is still within all PT_LOAD, we can probably do:

Expected<const uint8_t *> SecPtr = ElfFile.toMappedAddr(EntAddr);
Expected<const uint8_t *> SecEndPtr = ElfFile.toMappedAddr(EntAddr + DynEnt.StrSize);

And check if SecPtr and SecEndPtr contains any errors. Does that looks OK to you?

[mcgrathr]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

382

That would be better than nothing, but still susceptible to integer overflow. If there is nothing more robust than this that is easy to do without further refactoring, this seems like a wise incremental improvement. I think the long-term solution will indeed be to change this API to return some span-like type rather than a pointer so that accesses relative to the base address can be verified safe.

[haowei]

Adds minimal validation of STRSZ from .dynamic.

[jhenderson]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

408–412

I think you need a test case for this new error.

[haowei]

Test added in https://reviews.llvm.org/D119741

llvm/lib/InterfaceStub/ELFObjHandler.cpp

for (const IFSSymbol &Sym : Stub.Symbols) {
       DynSym.Content.add(DynStr.Content.getOffset(Sym.Name), Sym.Size, Bind,
                          convertIFSSymbolTypeToELF(Sym.Type), 0, Shndx);
     }
     DynSym.Size = DynSym.Content.getSize();
 
     // Poplulate dynamic table.
     size_t DynSymIndex = DynTab.Content.addAddr(DT_SYMTAB, 0);
     size_t DynStrIndex = DynTab.Content.addAddr(DT_STRTAB, 0);
     DynTab.Content.addValue(DT_STRSZ, DynSym.Size);
     for (const std::string &Lib : Stub.NeededLibs)
       DynTab.Content.addValue(DT_NEEDED, DynStr.Content.getOffset(Lib));
     if (Stub.SoName)
       DynTab.Content.addValue(DT_SONAME,
                               DynStr.Content.getOffset(Stub.SoName.getValue()));
     DynTab.Size = DynTab.Content.getSize();
     // Calculate sections' addresses and offsets.
     uint64_t CurrentOffset = sizeof(Elf_Ehdr);
private:
   uint64_t shdrOffset(const OutputSection<ELFT> &Sec) const {
     return ElfHeader.e_shoff + Sec.Index * sizeof(Elf_Shdr);
   }
 
   void writeShdr(uint8_t *Data, const OutputSection<ELFT> &Sec) const {
     write(Data + shdrOffset(Sec), Sec.Shdr);
   }
 };
+
+/// This function takes an error, and appends a string of text to the end of
+/// that error. Since "appending" to an Error isn't supported behavior of an
+/// Error, this function technically creates a new error with the combined
+/// message and consumes the old error.
+///
+/// @param Err Source error.
+/// @param After Text to append at the end of Err's error message.
+Error appendToError(Error Err, StringRef After) {
+  std::string Message;
+  raw_string_ostream Stream(Message);
+  Stream << Err;
+  Stream << " " << After;
+  consumeError(std::move(Err));
+  return createError(Stream.str());
+}
+
+template <class ELFT> class DynSym {
+  using Elf_Shdr_Range = typename ELFT::ShdrRange;
+  using Elf_Shdr = typename ELFT::Shdr;
+
+public:
+  static Expected<DynSym> create(const ELFFile<ELFT> &ElfFile,
+                                 const DynamicEntries &DynEnt) {
+    Expected<Elf_Shdr_Range> Shdrs = ElfFile.sections();
+    if (!Shdrs)
+      return Shdrs.takeError();
+    return DynSym(ElfFile, DynEnt, *Shdrs);
+  }
+
+  Expected<const uint8_t *> getDynSym() {
+    if (DynSymHdr)
+      return ElfFile.base() + DynSymHdr->sh_offset;
+    return getDynamicData(DynEnt.DynSymAddr, "dynamic symbol table");
+  }
+
+  // StringRef or uint8_t* return according to taste as to where the casts go.
+  Expected<StringRef> getDynStr() {
+    if (DynSymHdr)
+      return ElfFile.getStringTableForSymtab(*DynSymHdr, Shdrs);
+    Expected<const uint8_t *> DataOrErr =
+        getDynamicData(DynEnt.StrTabAddr, "dynamic string table");
+    if (!DataOrErr)
+      return DataOrErr.takeError();
+    return StringRef(reinterpret_cast<const char *>(*DataOrErr),
+                     DynEnt.StrSize);
+  }
+
+private:
+  DynSym(const ELFFile<ELFT> &ElfFile, const DynamicEntries &DynEnt,
+         Elf_Shdr_Range Shdrs)
+      : ElfFile(ElfFile), DynEnt(DynEnt), Shdrs(Shdrs),
+        DynSymHdr(findDynSymHdr()) {}
+
+  const Elf_Shdr *findDynSymHdr() {
+    for (const Elf_Shdr &Sec : Shdrs)
+      if (Sec.sh_type == SHT_DYNSYM)
+        return &Sec;
+    return nullptr;
+  }
+
+  Expected<const uint8_t *> getDynamicData(uint64_t EntAddr, StringRef Name) {
+    Expected<const uint8_t *> SecPtr = ElfFile.toMappedAddr(EntAddr);
+    if (!SecPtr)
+      return appendToError(SecPtr.takeError(), "when locating " +
+                                                   std::string(Name) +
+                                                   " section contents");
+    return *SecPtr;
+  }
+
+  const ELFFile<ELFT> &ElfFile;
+  const DynamicEntries &DynEnt;
+  Elf_Shdr_Range Shdrs;
+  const Elf_Shdr *DynSymHdr;
+};
 } // end anonymous namespace
 
 /// This function behaves similarly to StringRef::substr(), but attempts to
 /// terminate the returned StringRef at the first null terminator. If no null
 /// terminator is found, an error is returned.
 ///
 /// @param Str Source string to create a substring from.
 /// @param Offset The start index of the desired substring.
 static Expected<StringRef> terminatedSubstr(StringRef Str, size_t Offset) {
   size_t StrEnd = Str.find('\0', Offset);
   if (StrEnd == StringLiteral::npos) {
     return createError(
         "String overran bounds of string table (no null terminator)");
   }
 
   size_t StrLen = StrEnd - Offset;
   return Str.substr(Offset, StrLen);
 }
 
-/// This function takes an error, and appends a string of text to the end of
-/// that error. Since "appending" to an Error isn't supported behavior of an
-/// Error, this function technically creates a new error with the combined
-/// message and consumes the old error.
-///
-/// @param Err Source error.
-/// @param After Text to append at the end of Err's error message.
-Error appendToError(Error Err, StringRef After) {
-  std::string Message;
-  raw_string_ostream Stream(Message);
-  Stream << Err;
-  Stream << " " << After;
-  consumeError(std::move(Err));
-  return createError(Stream.str());
-}
-
 /// This function populates a DynamicEntries struct using an ELFT::DynRange.
 /// After populating the struct, the members are validated with
 /// some basic correctness checks.
 ///
 /// @param Dyn Target DynamicEntries struct to populate.
 /// @param DynTable Source dynamic table.
 template <class ELFT>
 static Error populateDynamic(DynamicEntries &Dyn,

 }
 
 /// Returns a new IFSStub with all members populated from an ELFObjectFile.
 /// @param ElfObj Source ELFObjectFile.
 template <class ELFT>
 static Expected<std::unique_ptr<IFSStub>>
 buildStub(const ELFObjectFile<ELFT> &ElfObj) {
   using Elf_Dyn_Range = typename ELFT::DynRange;
-  using Elf_Phdr_Range = typename ELFT::PhdrRange;
   using Elf_Sym_Range = typename ELFT::SymRange;
   using Elf_Sym = typename ELFT::Sym;
   std::unique_ptr<IFSStub> DestStub = std::make_unique<IFSStub>();
   const ELFFile<ELFT> &ElfFile = ElfObj.getELFFile();
   // Fetch .dynamic table.
   Expected<Elf_Dyn_Range> DynTable = ElfFile.dynamicEntries();
   if (!DynTable) {
     return DynTable.takeError();
   }
 
-  // Fetch program headers.
-  Expected<Elf_Phdr_Range> PHdrs = ElfFile.program_headers();
-  if (!PHdrs) {
-    return PHdrs.takeError();
-  }
-
   // Collect relevant .dynamic entries.
   DynamicEntries DynEnt;
   if (Error Err = populateDynamic<ELFT>(DynEnt, *DynTable))
     return std::move(Err);
+  Expected<DynSym<ELFT>> EDynSym = DynSym<ELFT>::create(ElfFile, DynEnt);
+  if (!EDynSym)
+    return EDynSym.takeError();
+
+  Expected<StringRef> EDynStr = EDynSym.get().getDynStr();
+  if (!EDynStr)
+    return EDynStr.takeError();
 
-  // Get pointer to in-memory location of .dynstr section.
-  Expected<const uint8_t *> DynStrPtr = ElfFile.toMappedAddr(DynEnt.StrTabAddr);
-  if (!DynStrPtr)
-    return appendToError(DynStrPtr.takeError(),
-                         "when locating .dynstr section contents");
-
-  StringRef DynStr(reinterpret_cast<const char *>(DynStrPtr.get()),
-                   DynEnt.StrSize);
+  StringRef DynStr = EDynStr.get();
 
   // Populate Arch from ELF header.
   DestStub->Target.Arch = static_cast<IFSArch>(ElfFile.getHeader().e_machine);
   DestStub->Target.BitWidth =
       convertELFBitWidthToIFS(ElfFile.getHeader().e_ident[EI_CLASS]);
   DestStub->Target.Endianness =
       convertELFEndiannessToIFS(ElfFile.getHeader().e_ident[EI_DATA]);
   DestStub->Target.ObjectFormat = "ELF";
 
   // Populate SoName from .dynamic entries and dynamic string table.
   if (DynEnt.SONameOffset.hasValue()) {
     Expected<StringRef> NameOrErr =
         terminatedSubstr(DynStr, *DynEnt.SONameOffset);
     if (!NameOrErr) {
       return appendToError(NameOrErr.takeError(), "when reading DT_SONAME");
     }
     DestStub->SoName = std::string(*NameOrErr);
   }
 
   // Populate NeededLibs from .dynamic entries and dynamic string table.
   for (uint64_t NeededStrOffset : DynEnt.NeededLibNames) {
     Expected<StringRef> LibNameOrErr =
         terminatedSubstr(DynStr, NeededStrOffset);
     if (!LibNameOrErr) {
       return appendToError(LibNameOrErr.takeError(), "when reading DT_NEEDED");
     }
     DestStub->NeededLibs.push_back(std::string(*LibNameOrErr));
   }
 
   // Populate Symbols from .dynsym table and dynamic string table.
   Expected<uint64_t> SymCount = ElfFile.getDynSymtabSize();
   if (!SymCount)
     return SymCount.takeError();
   if (*SymCount > 0) {
     // Get pointer to in-memory location of .dynsym section.
-    Expected<const uint8_t *> DynSymPtr =
-        ElfFile.toMappedAddr(DynEnt.DynSymAddr);
+    Expected<const uint8_t *> DynSymPtr = EDynSym.get().getDynSym();
     if (!DynSymPtr)
       return appendToError(DynSymPtr.takeError(),
                            "when locating .dynsym section contents");
     Elf_Sym_Range DynSyms = ArrayRef<Elf_Sym>(
         reinterpret_cast<const Elf_Sym *>(*DynSymPtr), *SymCount);
     Error SymReadError = populateSymbols<ELFT>(*DestStub, DynSyms, DynStr);
     if (SymReadError)
       return appendToError(std::move(SymReadError),
                            "when reading dynamic symbols");
   }
 
   return std::move(DestStub);

llvm/test/tools/llvm-ifs/binary-read-bad-vaddr.test

- Type:     PT_LOAD
     FirstSec: .dynstr
     LastSec:  .dynamic
   - Type:     PT_DYNAMIC
     Flags:    [ PF_X, PF_R ]
     VAddr:    0x1008
     FirstSec: .dynamic
     LastSec:  .dynamic
 
-# CHECK: virtual address is not in any segment: 0x260 when locating .dynstr section contents
+# CHECK: virtual address is not in any segment: 0x260 when locating dynamic string table section contents

llvm/test/tools/llvm-ifs/ifs-elf-conversion.test

+## Test writing stub ELF from IFS and read stub ELF to regenerate IFS.
+
+# RUN: llvm-ifs --output-elf=%t.elf64l --arch=x86_64 --bitwidth=64 --endianness=little %s
+
+# RUN: llvm-ifs --output-ifs=- --strip-ifs-target %t.elf64l | FileCheck %s
+
+--- !ifs-v1
+IfsVersion: 3.0
+NeededLibs:
+  - libc.so.6
+Symbols:
+  - { Name: bar, Type: Object, Size: 42 }
+  - { Name: baz, Type: TLS, Size: 3 }
+  - { Name: plus, Type: Func }
+...
+
+# CHECK:      --- !ifs-v1
+# CHECK-NEXT: IfsVersion: 3.0
+# CHECK-NEXT: NeededLibs:
+# CHECK-NEXT:   - libc.so.6
+# CHECK-NEXT: Symbols:
+# CHECK-NEXT:   - { Name: bar, Type: Object, Size: 42 }
+# CHECK-NEXT:   - { Name: baz, Type: TLS, Size: 3 }
+# CHECK-NEXT:   - { Name: plus, Type: Func }
+# CHECK-NEXT: ...