[PATCH] D111358: TargetLibraryInfo checker tool

Tue Nov 23 14:56:19 PST 2021

probinson marked 5 inline comments as done.
probinson added a comment.

In D111358#3144735 <https://reviews.llvm.org/D111358#3144735>, @MaskRay wrote:

> 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?

I don't see any entry in https://llvm.org/docs/CommandGuide/index.html for llvm-ifs, and there is no description in llvm-ifs.cpp so it is hard to know what it does.  I do see comments about "emitting stubs" which is not at all what this tool does, so I'm inclined to say No, combining them is not suitable.

See D114478 <https://reviews.llvm.org/D114478> for code and test changes in response to this review.

================
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
----------------
jhenderson wrote:
> MaskRay wrote:
> > 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 
> +1 to what MaskRay said. If you need guidance on what the YAML might look like, feel free to chat with me outside this review.
> 
> You also can't use `lld` in a RUN command outside the `lld` and `cross-project-tests` projects, since there's no guarantee it would have been built (`llvm/tools` tests only have access to the core `llvm` project components).
> 
> Apologies for not noticing these points earlier: this slipped off my radar, and I ended up looking at other stuff.
I'll point out that the assembler source, with instructions for regenerating the prebuilt objects, is in the relanded patch, so there's no trouble to recreate them if necessary.

But I did get yaml2obj to do what I need, and the net file size is a small decrease, so I've replaced the test.

================
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.
----------------
MaskRay wrote:
> https://llvm.org/docs/CodingStandards.html#anonymous-namespaces
Fixed comment

================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:106
+  OutputName += "'";
+  if (Name.startswith("_Z") || Name.startswith("??")) {
+    OutputName += " aka ";
----------------
MaskRay wrote:
> What does `??` do? It's not regular external name prefix (`_Z`). If it is organization-specific, there should be a comment.
All names in the TLI set are external.  `_Z` indicates an Itanium-style mangled name; `??` is the prefix for Windows mangling.
I've added a comment to that effect.

================
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.
----------------
MaskRay wrote:
> This can just use free functions instead of inheriting from std::vector.
I think this is better OO design.

================
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);
----------------
MaskRay wrote:
> SDK may be an organization-specific term. The more widely used term is library.
SDK is a common industry term.  I didn't use "library" because the APIs may be implemented across several libraries within an SDK.

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

================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:173
+
+  for (auto I = ELF->getDynamicSymbolIterators().begin();
+       I != ELF->getDynamicSymbolIterators().end(); ++I) {
----------------
MaskRay wrote:
> range-based for
I thought I had tried that and it didn't work, but it does now; changed.

================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:189
+  int Index = -1;
+  for (auto &C : A->children(Err)) {
+    ++Index;
----------------
MaskRay wrote:
> llvm::enumerate
I tried that, but the iterator doesn't have a `reference` member that `llvm::enumerate` appears to need.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111358