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

Thu Dec 13 03:03:02 PST 2018

jhenderson added inline comments.

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-arch.test:3
+# RUN: llvm-elfabi %t -emit-tbe=%t2
+# RUN: cat %t2 | FileCheck %s
+
----------------
Why the cat command? You can have FileCheck read a file via --input-file (also applies to other tests).

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-arch.test:12
+
+#CHECK:         --- !tapi-tbe
+#CHECK-NEXT:    TbeVersion: {{[1-9]\d*\.(0|([1-9]\d*))}}
----------------
Nit here and elsewhere - remove all the extra spaces between the CHECK/CHECK-NEXT and the actual text being checked. (It's okay to have the spaces after CHECK: to make the patterns match with CHECK-NEXT, but this test has extra on top of that).

================
Comment at: llvm/test/tools/llvm-elfabi/fail-file-open.test:5
+
+#CHECK:    Error: Could not open `{{.*}}`.
----------------
Perhaps the error check here could include "NotAFileInTestingDir" at the end?

================
Comment at: llvm/test/tools/llvm-elfabi/replace-soname-tbe.test:2
+# RUN: yaml2obj %s > %t
+# RUN: llvm-elfabi %t --emit-tbe=%t2 -so-name=best.so
+# RUN: cat %t2 | FileCheck %s
----------------
Nit: inconsistent use of '--' and '-' for switch prefixes.

================
Comment at: llvm/test/tools/llvm-elfabi/tbe-read-basic.test:17
+#CHECK:         --- !tapi-tbe
+#CHECK-NEXT:    TbeVersion: {{[1-9]\d*\.(0|([1-9]\d*))}}
+#CHECK-NEXT:    SoName: somelib.so
----------------
Don't know if it belongs in this particular test, but I feel like we should have at least one test that shows the current version is correct.

================
Comment at: llvm/tools/LLVMBuild.txt:35
  llvm-dwp
+ llvm-elfabi
  llvm-exegesis
----------------
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.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:32
+Expected<std::unique_ptr<ELFStub>>
+buildStub(const object::ELFObjectFile<ELFT> &ElfObj) {
+  std::unique_ptr<ELFStub> DestStub = make_unique<ELFStub>();
----------------
Nit: you should be able to remove the object::, since you're using the namespace object.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:39
+  // .dynsym, and .dynstr.
+  const Elf_Ehdr *ElfHeader = ElfFile->getHeader();
+
----------------
Will the ElfHeader likely be used for anything else? If not, you should fetch it inline, I think.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:41
+
+  // Populate architecture.
+  Error ReadArchError = populateArch<ELFT>(*DestStub, *ElfHeader);
----------------
No need for this comment. It's obvious from the function name.

================
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;
----------------
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.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:28
+/// Returns a new ELFStub with all members populated from an ELFObjectFile.
+/// @param ElfObj Pointer to an ELFObjectFile as a source.
+template <class ELFT>
----------------
Pointer -> Reference.

The sentence as a whole sounds a bit clunky. Maybe "Source ELFObjectFile". I don't think you need to explicitly state that it's a pointer or reference (it's in the type signature).

================
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.
----------------
This comment smells a bit like this should be a member function of ELFStub. Thoughts on that?

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:34-35
+/// Populate the `Arch` member of an ELFStub.
+/// @param TargetStub Reference to an ELFStub who's member will be populated.
+/// @param Header Reference to an ELF file header.
+template <class ELFT>
----------------
who's -> whose

As above, I don't think you need to explicitly say that TargetStub and Header are references.

What is the ELF file header for however? That needs explaining more here.

================
Comment at: llvm/tools/llvm-elfabi/ErrorCollector.h:1
+//===- ErrorCollector.h -----------------------------------------*- C++ -*-===//
+//
----------------
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 ]].

================
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;
----------------
jakehehrlich wrote:
> 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. 
FWIW, in functions in the tool itself, I have no issue with errors raised within functions. I don't think it's always necessary to propagate back up to the top-level. The issue is when they are in library functions.

================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:31
+            cl::value_desc("path"));
+cl::opt<std::string> SetSoname(
+    "so-name",
----------------
I think the convention tends to be for cl::opt variable names to essentially match the switch name, so this should just be "Soname" (or "SoName", or even "SOName").

================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:32
+cl::opt<std::string> SetSoname(
+    "so-name",
+    cl::desc("Manually set the DT_SONAME entry of any emitted files."),
----------------
Perhaps this should be --soname to match the equivalent linker flag.

================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:39
+static void updateTBEVersion(ELFStub &Stub) {
+  Stub.TbeVersion = TBEVersionCurrent;
+}
----------------
Similar to populateArch, why is this a separate function and not just done inline (or possibly a member function of ELFStub)?

================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:43
+/// updateSoName() overrides the DT_SONAME for a stub as specified by the user.
+static void updateSoName(ELFStub &Stub) {
+  if (SetSoname.getNumOccurrences() == 1) {
----------------
Again, I'm not sure there's any value in this function being out-of-line.

================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:70
+static Expected<std::unique_ptr<ELFStub>> readInputFile(StringRef FilePath) {
+  ErrorCollector EC(/*UseFatalErrors=*/false);
+  // Read in file.
----------------
Move this down to first use.

================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:85
+  if (!ELFReadError) {
+    return std::move(StubFromELF.get());
+  }
----------------
I think `*StubFromELF` rather than `StubFromELF.get()` is more common usage in this case.

This could probably be written a bit simpler as follows:
```
Expected<std::unique_ptr<ELFStub>> StubFromELF =
      readELFFile(FileReadBuffer->getMemBufferRef());
if (StubFromELF) {
  return std::move(*StubFromELF);
}
EC.addError(StubFromELF.takeError(), "BinaryRead");
```

================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:111
+    errs() << "Error: " << ReadError << "\n";
+    errs() << "All file readers failed to read `" << InputFilePath.c_str()
+           << "`. (unsupported/malformed file?)\n";
----------------
This message sounds a bit clunky. Perhaps "No file readers were able to read..."?

================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:128
+  }
+  return 0;
+}
----------------
Nit: No need for explicit return 0 in a main in C++ these days.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55352