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

Thu Dec 13 15:11:13 PST 2018

amontanez marked 7 inline comments as done.
amontanez added a comment.

Hopefully this clarifies the direction on some things brought up. I didn't explicitly mention that everything outside `llvm-elfabi.cpp` is designed as if it were part of a library. Let me know if this resolves some concerns you had or brings up new ones. I really appreciate your insight, push back again on anything you still feels needs to be changed and I'll look into it again.

================
Comment at: llvm/tools/LLVMBuild.txt:35
  llvm-dwp
+ llvm-elfabi
  llvm-exegesis
----------------
jhenderson wrote:
> I'm not entirely convinced by the "elfabi" name since it isn't really an ABI, but I don't have a clearly better name.
That's very understandable. We didn't have any better ideas for names, so we went with elfabi. I'm totally open to suggestions since I understand changing this later would be a huge pain.

This tool views ELF shared objects roughly from a compile-time linker's perspective, so maybe there's a better name to be had that stems from that.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:39
+  // .dynsym, and .dynstr.
+  const Elf_Ehdr *ElfHeader = ElfFile->getHeader();
+
----------------
jhenderson wrote:
> Will the ElfHeader likely be used for anything else? If not, you should fetch it inline, I think.
Done. I don't think we'll need to explicitly use it again.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:54
+template <class ELFT>
+Error populateArch(ELFStub &TargetStub, const typename ELFT::Ehdr &Header) {
+  TargetStub.Arch = Header.e_machine;
----------------
jhenderson wrote:
> Are you anticipating this function becoming more complicated? At the moment, there's no point in it returning an error, or even really existing, since you could just set the TargetStub.Arch member at the call site instead.
I've changed it to `void` as I doubt this will become more complicated over time.

Even though it is very simple, I would like to keep it as a function. The plan is to have one function for each ELFStub member. `buildStub()` streamlines the process of fetching the arguments needed for each function from an `ELFFile<ELFT>`, but I don't want the implementation of `buildStub()` to be the sole point of documentation on how to populate `ELFStub.Arch`. I'd like it to be very clear what information is required to populate each member. While this is simple for `Arch`, it becomes a little more involved for some of the other members. having `populateArch()` helps with consistency in this situation. D55629 helps to better illustrate the direction I'd like to take.

This is also a result of trying to design a consistent, unit-testable interface. If you depend on using complete `ELFObjectFile`s, things become significantly harder to unit test. I understand I can't unit test a tool, but that was how I originally designed it, and would be useful if this does get moved to a library.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:33
+
+/// Populate the `Arch` member of an ELFStub.
+/// @param TargetStub Reference to an ELFStub who's member will be populated.
----------------
jhenderson wrote:
> This comment smells a bit like this should be a member function of ELFStub. Thoughts on that?
While that is definitely an appealing approach, we've committed to not having the TextAPI library depend on libObject (Juergen would like libObject to depend on some parts of the MachO TextAPI implementation). The result of that discussion was that the ELF implementation for TextAPI will mostly just contain the YAML reader/writer and the ELFStub internal representation for now.

Overall, everything in this tool outside of llvm-elfabi.cpp is designed as if it were part of a library. There's two main reasons for this: 

1. It's not completely clear where the ELF binary readers/writers belong since they won't be in TextAPI. An argument could be made that they should be in a library, but a counter argument could be that binary readers/writers are only really used for the tool. I'd rather put it in the tool for now to avoid bikeshedding another library. That allows me to start adding stubbing functionality asap.

2. Designing everything as if it were part of a library prevents this tool from becoming a monolith that can't later be turned into a library.

================
Comment at: llvm/tools/llvm-elfabi/ErrorCollector.h:1
+//===- ErrorCollector.h -----------------------------------------*- C++ -*-===//
+//
----------------
jhenderson wrote:
> My understanding of `Error` is that you don't need this class. `Error` can actually represent multiple errors. You can use `joinErrors()` to combine two `Error` instances into a single one. See [[ https://llvm.org/docs/ProgrammersManual.html#concatenating-errors-with-joinerrors | the docs here ]].
I looked into that initially, and I don't feel it solves our problem. While joinErrors() solves the problem of collecting multiple errors into one, `ErrorCollector` solves our issue of determining whether or not to consume errors.

In `llvm-elfabi.cpp`, each file reader might fail and produce an error. If *any* reader succeeds, though, the other errors should be consumed as they become irrelevant. If none succeed, they should all be reported (in the future Jake and I would like to have more fine grained control over which are reported, but that's not in the scope of this patch). This was the cleanest way I felt we could resolve the issue without having very messy use of consumeError() throughout `readInputFile()`.

================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:39
+static void updateTBEVersion(ELFStub &Stub) {
+  Stub.TbeVersion = TBEVersionCurrent;
+}
----------------
jhenderson wrote:
> Similar to populateArch, why is this a separate function and not just done inline (or possibly a member function of ELFStub)?
You're right, there's not much need for `updateTBEVersion()`. I could see there being more logic in the future if there's a desire to write a TBE using an older writer, but it probably doesn't warrant a function right now. I've inlined it in main().

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55352