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

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added a comment.

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.



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:529
+  }
+  // Fallback to use offsets from .dynamic as .dynsym was not found
+  // from the section headers.
----------------
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`.



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:535
+  }
+  if (PHdrs.get().size() > 0) {
+    Expected<const uint8_t *> DynStrPtr =
----------------
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).



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:570
+  }
+  // Fallback to use offsets from .dynamic as .dynsym was not found
+  // from the section headers.
----------------
Same here wrt the fallback logic.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:573
+  Expected<Elf_Phdr_Range> PHdrs = ElfFile.program_headers();
+  if (!PHdrs) {
+    return PHdrs.takeError();
----------------
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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116769