[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:374
+
+  // StringRef or uint8_t* return according to taste as to where the casts go.
+  Expected<StringRef> getDynStr() {
----------------
Sorry, this comment was intended purely for illustration of my example! You don't need it in the real code.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:402-404
+      return appendToError(SecPtr.takeError(), "when locating " +
+                                                   std::string(Name) +
+                                                   " section contents");
----------------
`StringRef` + string literals converts to `llvm::Twine`. Prefer that for concatenation and then use `.str()` to convert back to a `std::string` afterwards, as it's more efficient.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:588
 
-  // Get pointer to in-memory location of .dynstr section.
-  Expected<const uint8_t *> DynStrPtr = ElfFile.toMappedAddr(DynEnt.StrTabAddr);
-  if (!DynStrPtr)
-    return appendToError(DynStrPtr.takeError(),
-                         "when locating .dynstr section contents");
+  Expected<StringRef> EDynStr = EDynSym.get().getDynStr();
+  if (!EDynStr)
----------------
I think you can just do the inline edit.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:592
 
-  StringRef DynStr(reinterpret_cast<const char *>(DynStrPtr.get()),
-                   DynEnt.StrSize);
+  StringRef DynStr = EDynStr.get();
 
----------------
I'm pretty sure this is more idiomatic.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:628
     // Get pointer to in-memory location of .dynsym section.
-    Expected<const uint8_t *> DynSymPtr =
-        ElfFile.toMappedAddr(DynEnt.DynSymAddr);
+    Expected<const uint8_t *> DynSymPtr = EDynSym.get().getDynSym();
     if (!DynSymPtr)
----------------



================
Comment at: llvm/test/tools/llvm-ifs/ifs-elf-conversion.test:3-5
+# RUN: llvm-ifs --output-elf=%t.elf64l --arch=x86_64 --bitwidth=64 --endianness=little %s
+
+# RUN: llvm-ifs --output-ifs=- --strip-ifs-target %t.elf64l | FileCheck %s
----------------
As the test is the sequence of the two commands, rather than the two commands being independent, I think the blank line between them should be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116769