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

Wed Jan 12 15:04:41 PST 2022

haowei marked 5 inline comments as done.
haowei added inline comments.

================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:523
+        // If multiple .dynsym presents, uses the first one.
+        // This behavior aligns with llvm::object::ELFFile::getDynSymtabSize()
+        return ElfFile.getStringTableForSymtab(Sec, *Shdrs);
----------------
jhenderson wrote:
> 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.
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. 

================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:540
+template <class ELFT>
+static Expected<const uint8_t *> getDynSymPtr(const ELFFile<ELFT> &ElfFile,
+                                              const DynamicEntries &DynEnt) {
----------------
jhenderson wrote:
> Similar note to above, maybe this should be part of the `ElfFile` interface?
See the explanation above.

================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:546
+  Expected<Elf_Shdr_Range> Shdrs = ElfFile.sections();
+  if (!Shdrs) {
+    return Shdrs.takeError();
----------------
jhenderson wrote:
> Same as above re. braces, here and below.
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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116769