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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:552
+      if (Sec.sh_type == ELF::SHT_DYNSYM) {
+        // If multiple .dynsym present, use the first one.
+        // This behavior aligns with llvm::object::ELFFile::getDynSymtabSize()
----------------
Ditto (additional "are" needed).


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:522
+      if (Sec.sh_type == ELF::SHT_DYNSYM) {
+        // If multiple .dynsym presents, uses the first one.
+        // This behavior aligns with llvm::object::ELFFile::getDynSymtabSize()
----------------
jhenderson wrote:
> 
You missed the additional "are".


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:549
+  }
+  if (Shdrs.get().size()) {
+    for (const Elf_Shdr &Sec : *Shdrs) {
----------------
jhenderson wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116769/new/

https://reviews.llvm.org/D116769