[PATCH] D114478: [TLI checker] Update for post-commit review comments

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml:9
+# RUN: yaml2obj %s -DZDAPV=_ZdaPvj -o=%t2
+# RUN: echo %t2 > %t2.txt
+# RUN: llvm-tli-checker --triple x86_64-scei-ps4 @%t2.txt | \
----------------
I might be missing something obvious, but what's the point of this `echo`? Can't the object be passed directly on the command-line?


================
Comment at: llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml:53-56
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x1C8
+    Link:            .dynstr
+    AddressAlign:    0x8
----------------
How important are these fields for the tool? Are they actually needed, or could you get away without them for the purpose of this test?

(I believe the `Link: .dynstr` is implicit, so you should be able to omit that regardless)

Same question for the other sections.


================
Comment at: llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml:70-73
+    Type:            STT_FUNC
+    Section:         .text
+    Binding:         STB_GLOBAL
+    Value:           0x3D50
----------------
Are the Value and Type fields important to the tool?

Also, do you need this many symbols (it's fine if you do, just is a lot!)?


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:107
+  // Append a friendlier version of Itanium or Windows mangled names.
   if (Name.startswith("_Z") || Name.startswith("??")) {
     OutputName += " aka ";
----------------
Is this check strictly appropriate?

# There are efforts to add D and Rust demangling schemas to the set of supported functionality in the LLVM demangler, and this tool would not work with them.
# It seems like you should just attempt to call `demangle` and check to see if the output is different to the input string, to determine whether the name is actually demangled. Otherwise, you could end up with input symbol names that aren't valid (or aren't supported by the demangler) reading something like this "_Zsomeinvalidstring aka _Zsomeinvalidstring", which probably isn't what you want. If you really don't want to even attempt to demangle something (or you only want to support Itanium and Microsoft mangling schemes), you should probably make the is*Encoding functions in demangle.cpp into external functions that you can reference here.


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:169
   if (!O->isELF()) {
-    WithColor::warning() << "Only ELF-format files are supported\n";
+    WithColor::warning() << "only ELF-format files are supported\n";
     return;
----------------
If the tool can take more than one input (I haven't checked if it can), it might be wise to include the filename in this message.


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

https://reviews.llvm.org/D114478