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

[Armando Montanez via Phabricator via llvm-commits]

amontanez marked 2 inline comments as done.
amontanez added inline comments.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:61
+  } else if (Stub.SoName.length() == 0) {
+    Stub.SoName = FileName;
+    Stub.SoName.append(".so");
----------------
jakehehrlich wrote:
> As you might have heard me just speak with Roland the SoName should be optional and never synthetic.
I've noted this and will make SoName optional in a separate patch. For now I have removed the code that would generate a SoName from the file name if the SoName was empty.


================
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)
----------------
jakehehrlich wrote:
> 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.
If the YAML reader fails, it will always output to stderr. Having the YAML reader second causes this to only occur if both fail. I can later augment llvm::yaml::Input so we can have more control over where YAML outputs the error to, but for now I've defaulted to outputting both errors in the situation that an unreadable file is encountered. This makes slightly more sense given that right now I can't completely filter out the YAML error if the heuristics determine the binary error is of more interest.

This function's implementation isn't scalable, but it also doesn't impact design decisions of other code in this tool. I have marked a todo so I can revisit this separately. Let me know if you feel I should address this immediately.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55352