[PATCH] D55839: [elfabi] Add support for writing ELF header for binary stubs

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:72
+      FileOutputBuffer::create(FilePath, getBinaryStubSize<ELFT>(Stub));
+  if (!BufOrError) {
+    return createStringError(errc::invalid_argument,
----------------
Since you have an error and are currently returning an error, that error should incorporate the specific information from this error and not make it more vauge.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:185-186
+  if (BinaryOutputFilePath.getNumOccurrences() == 1) {
+    Error BinaryWriteError =
+        writeBinaryWithTarget(BinaryOutputFilePath, *TargetStub);
+    if (BinaryWriteError) {
----------------
ruiu wrote:
> It might be discussed before, but what is a value of returning an Error from a function and then print that error & exit from the main function? Propagating an error all the way to the main function makes function signatures more complex (any function that can fail or calls a function that can fail has to have a function signature of ErrorOr<T> instead of just T). If this is just a command, then maybe just printing out an error message and exit is better?
Yeah there's some line where things will only ever go in this code and some line where things would go into an eventual library. It isn't 100% clear where that line is right now. I'm in favor of earing on the side of caution and propagating more than less for the time being.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55839