[PATCH] D101835: [llvm] llvm-tapi-diff

[Cyndy Ishida via Phabricator via llvm-commits]

cishida added inline comments.


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:1
+//===-- llvm-tapi-diff.cpp - symbol table comparator command-line driver ---*-
+// C++
----------------
we should also replace `symbol table comparator` with something like `(tapi|tbd) file comparator`


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:11
+//
+// This file defines the command-line driver for the llvm-nm difference engine.
+//
----------------
s/llvm-nm/llvm-tapi-diff


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:79
+  auto BinRHS = convertFileToBinary(InputFileNameRHS);
+  if (!BinLHS || !BinRHS)
+    return EXIT_FAILURE;
----------------
Please add a testcase that goes through an error path. 
When I ran the tool with a misspelled input it crashes. 
If I understood @JDevlieghere comment correctly, we should be handling the error at this call site instead of inside `convertFileToBinary` and currently we don't capture the possible error from the wrapped `Expected<T>` object. 

```bin/llvm-tapi-diff: error: src/llvm-project/llvm/test/tools/llvm-tapi-diff/Inputs/vC.tbd: No such file or directory
Expected<T> must be checked before access or destruction.
Unchecked Expected<T> contained error:
No such file or directoryPLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: bin/llvm-tapi-diff  src/llvm-project/llvm/test/tools/llvm-tapi-diff/Inputs/v4B.tbd  src/llvm-project/llvm/test/tools/llvm-tapi-diff/Inputs/vC.tbd
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  llvm-tapi-diff           0x00000001084d4dd7 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 39
1  llvm-tapi-diff           0x00000001084d3ff8 llvm::sys::RunSignalHandlers() + 248
<omitted>
[1]    85533 abort      bin/llvm-tapi-diff
```

See https://llvm.org/docs/ProgrammersManual.html#error-handling for more details. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101835