[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
+template <class ELFT> void ELFStubBuilder<ELFT>::build() {
+  if (Bin == nullptr)
+    terminateWithError(
----------------
If something should never be null, you generally should use a reference. If you can't use a reference it is often (but not always) the case that something is wrong with the design. I generally prefer references to pointers.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:48
+
+std::unique_ptr<ELFStub> ELFObjReader::readFile(MemoryBufferRef Buf) {
+  std::unique_ptr<ELFStub> DestStub = make_unique<ELFStub>();
----------------
Yeah I think this should just be a single function and the other classes shouldn't exist.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:52
+  if (!BinOrErr) {
+    consumeError(BinOrErr.takeError());
+    return nullptr;
----------------
Don't consume the error, return it. Also I do this awful thing throughout llvm-objcopy where I just hard fail the program and exit as soon as something bad happens...you haven't done that but I'd like restress that you shouldn't do that.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:71
+    terminateWithError(createStringError(errc::not_supported,
+                                         "Unsupported ELF file architecture."));
+  }
----------------
technically it could just be any binary format you didn't expect. Also this should probably return an Expected unique pointer that is never null.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:34
+/// called by directly providing the needed information.
+template <class ELFT> class ELFStubBuilder {
+public:
----------------
Can you just make this a function? I don't think it needs a whole class at the moment and I'm not sure it ever will.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:43
+               : Stub(EmptyStub), Bin(ElfObj) {}
+  ELFStubBuilder(ELFStub &EmptyStub) : Stub(EmptyStub), Bin(nullptr) {}
+
----------------
I don't think this interface makes sense. What does constructing an ELFStubBuilder without an ELF object file accomplish? Also if we 100% expect this to be empty, maybe the interface that is used should return a new ELFStub rather than requiring that we give it an empty one.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:57
+
+class ELFObjReader {
+public:
----------------
It doesn't seem like this should be a class. It contains no state and has one function. Functions are your friend, unnecessary classes are the enemy.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:24-25
+
+TBEHandler TextHandler;
+ELFObjReader BinaryReader;
+
----------------
Having this as global is bad. Globals (sans use cases like options) mean one of two things 1) you shouldn't have the class in the first place 2) you're maintaining global state which is almost always bad.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:61
+  } else if (Stub.SoName.length() == 0) {
+    Stub.SoName = FileName;
+    Stub.SoName.append(".so");
----------------
As you might have heard me just speak with Roland the SoName should be optional and never synthetic.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:68
+/// of the YAML parser.
+static void writeTBE(StringRef FilePath, ELFStub &Stub) {
+  std::error_code SysErr;
----------------
Return an error rather than terminating even here. This is an example of that terrible thing that I did. I also let that reality seep into all the nooks and crannies of my interfaces. 


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:79
+  // Update SoName for output files.
+  updateSoName(Stub, sys::path::stem(FilePath));
+  Stub.TbeVersion = TBEVersionCurrent;
----------------
writing should just wirte, not modify. If you update the ELFStub it should go 1) Modify 2) Write.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:96
+  // Read in file.
+  std::unique_ptr<ELFStub> Stub;
+  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrError =
----------------
Wait to declare this until you construct it later.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:100
+  if (!BufOrError) {
+    return createStringError(BufOrError.getError(), "unable to read file");
+  }
----------------
I think "unable to read file" is vague and likely covered by error already. Maybe "unable to read .tbe file '%s'" where "%s" should be the filename even if the filename might be mentioned in the error. That gives more information.

You should have a test to confirm that this error happens and is handled correctly.

Also do you want to take the error here?


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:106
+  // First try to read as a binary (fails fast if not binary).
+  Stub = BinaryReader.readFile(FileReadBuffer->getMemBufferRef());
+  if (Stub.get() != nullptr)
----------------
As discussed offline, we should use Expected, check for an error (and try the next if there is one) and then create a new Error that wraps both and then chooses which one to report based on some heuristic (basically move the heuristic to the error reporting rather than the file parsing stage so that no heuristic is ever used for parsing). I know I said checking the extension would be easy but checking for ELF magic is also probably easy and more reliable so I guess now I agree with you (I was just thinking it would be hard to check the magic here). It will probably make sense to also include the file path in the error that you create so that it can report which file the error was an issue for. A really cool heuristic would be

1) If the first 4 bytes of the memory are ELF magic, report the ELF error (but state assumption)
2) If the extension is .tbe report the the TBE error (but state the assumption). Also you can search the file for 
3) If the neither of those things are true, tell the user you don't know what's going on and report both. It's also acceptable to search the whole file looking for a sign that

If you'd want I'd also still accept just looking at the extension.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:121
+  cl::ParseCommandLineOptions(argc, argv);
+  std::unique_ptr<ELFStub> TargetStub;
+
----------------
Don't declare until you construct later.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:125
+  if (!StubOrErr) {
+    errs() << "Failed to load input file:\n";
+    terminateWithError(StubOrErr.takeError());
----------------
That should probably be a part of the error already if possible.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.h:22
+
+LLVM_ATTRIBUTE_NORETURN void terminateWithError(std::error_code Err);
+LLVM_ATTRIBUTE_NORETURN void terminateWithError(Error Err);
----------------
I realize I do this in llvm-objcopy but I do it so that I can exit with an error from wherever. It's really better to propagate errors all the way back to main or near main and handle them once there if possible. That might not be a perfect fit but if you find that you *can* plumb all the errors back to main, you should remove these functions and this file.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55352