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

[Haowei Wu via Phabricator via llvm-commits]

haowei marked an inline comment as done.
haowei added inline comments.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:549
+  }
+  if (Shdrs.get().size()) {
+    for (const Elf_Shdr &Sec : *Shdrs) {
----------------
jhenderson wrote:
> 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.
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](https://gist.github.com/zeroomega/9552161efca45ad8af17a3f312582753#file-lambda_solution-cpp-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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116769