[PATCH] D55352: [elfabi] Introduce tool for ELF TextAPI

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:29
+Expected<std::unique_ptr<ELFStub>>
+llvm::elfabi::buildStub(const object::ELFObjectFile<ELFT> &ElfObj) {
+  std::unique_ptr<ELFStub> DestStub = make_unique<ELFStub>();
----------------
nit: You can just put all these inside of llvm::elfabi namespace rather than repeatedly declaring them as such.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:42
+    return std::move(ObjectFileReadError);
+
+  return std::move(DestStub);
----------------
nit: Can you add some rough TODOs here on what else you need to add to get this fully featured?


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:61
+  if (auto Obj = dyn_cast<ELFObjectFile<ELF32LE>>(Bin)) {
+    return buildStub<ELF32LE>(*Obj);
+  } else if (auto Obj = dyn_cast<ELFObjectFile<ELF64LE>>(Bin)) {
----------------
nit: The type should be automatically inferred so you don't need the explicit template parameter.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:36-44
+LLVM_ATTRIBUTE_NORETURN void terminateWithError(std::error_code Err) {
+  errs() << "Error: " << Err.message() << "\n";
+  exit(1);
+}
+
+LLVM_ATTRIBUTE_NORETURN void terminateWithError(Error Err) {
+  errs() << "Error: " << Err << "\n";
----------------
nit: Can you just inline these and return from main instead of calling these? I think we don't expect any other exit points. Having these functions around is just kind of asking for them to be used and I'm skeptical that's a good idea.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:120-131
+  // If both readers fail, build a new error that includes all information.
+  std::string ReaderErrors;
+  raw_string_ostream ErrorCollector(ReaderErrors);
+  ErrorCollector << "Binary read error: " << ELFReadError << "\n";
+  ErrorCollector << "YAML parse error: " << TBEReadError << "\n";
+  consumeError(std::move(TBEReadError));
+  consumeError(std::move(ELFReadError));
----------------
I think this should be implemented as a new aggregate error that you return. You can move the errors into the aggregate error and then the main function can report the aggregate error.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55352