[PATCH] D111358: TargetLibraryInfo checker tool

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

What this tool does fit a bit into the realm of llvm-ifs (it had a previous name "llvm-elfabi" D100139 <https://reviews.llvm.org/D100139>. I like "elfabi" better as "ifs" seems confusing). Would it be suitable to move the code there?



================
Comment at: llvm/test/tools/llvm-tli-checker/ps4-tli-check.s:8
+# RUN: llvm-mc --triple=x86_64-scei-ps4 --defsym WRONG=1 --filetype=obj %s -o %t2.o
+# RUN: ld.lld --shared %t2.o -o %t2.so
+# RUN: echo %t2.so > %t2.txt
----------------
The relanded version (38be8f4057c1bf19fd02d08d6116e28983a49d8d) uses prebuilt shared objects `Inputs/ps4-tli-check.right.so` and `Inputs/ps4-tli-check.wrong.so`. For llvm/tools/* tools working on object files, we try very hard to avoid checking in new prebuilt files: they are opaque, difficult to update, and waste space.

It's recommended to use yaml2obj, like many tests in test/tools/llvm-objcopy and test/tools/llvm-readobj.

@jhenderson 


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:61
+// We have three levels of reporting.
+enum class ReportKind {
+  Error,       // For argument parsing errors.
----------------
https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:70
+// to ignore those.
+template <typename T> T unwrapIgnoreError(Expected<T> E) {
+  if (E)
----------------
static


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:106
+  OutputName += "'";
+  if (Name.startswith("_Z") || Name.startswith("??")) {
+    OutputName += " aka ";
----------------
What does `??` do? It's not regular external name prefix (`_Z`). If it is organization-specific, there should be a comment.


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:116
+// This is a vector to preserve the sorted order for better reporting.
+struct TLINameList : std::vector<std::pair<StringRef, bool>> {
+  // Record all the TLI info in the vector.
----------------
This can just use free functions instead of inheriting from std::vector.


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:122
+};
+TLINameList TLINames;
+
----------------
static


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:155
+// We use a map to get hashed lookup speed; the bool is meaningless.
+class SDKNameMap : public StringMap<bool> {
+  void populateFromObject(ObjectFile *O);
----------------
SDK may be an organization-specific term. The more widely used term is library.


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:162
+};
+SDKNameMap SDKNames;
+
----------------
static


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:168
+  if (!O->isELF()) {
+    WithColor::warning() << "Only ELF-format files are supported\n";
+    return;
----------------
I know that the file has settled on capitalized messages, but non-capitalized message are more common:
https://llvm.org/docs/CodingStandards.html#error-and-warning-messages


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:171
+  }
+  auto *ELF = cast<const ELFObjectFileBase>(O);
+
----------------
More common to omit `const` for `cast` and change `auto *` to `const auto *`


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:173
+
+  for (auto I = ELF->getDynamicSymbolIterators().begin();
+       I != ELF->getDynamicSymbolIterators().end(); ++I) {
----------------
range-based for


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:189
+  int Index = -1;
+  for (auto &C : A->children(Err)) {
+    ++Index;
----------------
llvm::enumerate


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:221
+    // FIXME: Report this better.
+    WithColor::defaultWarningHandler(ExpectedBinary.takeError());
+    return;
----------------
It is more common to define a warning function and avoid referencing `WithColor` in the main program.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111358