[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:516-518
+  if (!Shdrs) {
+    return Shdrs.takeError();
+  }
----------------
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.



================
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()
----------------



================
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);
----------------
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.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:536
+
+/// Returns the in-memory pointer to the DynSym from an ElfFile
+/// @param ElfFile Reference to the ELFFile object.
----------------
I'm not sure if the doxygen style comments are really needed on `static` functions, but I'm not necessarily opposed to them.


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


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116769