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

[Paul Robinson via Phabricator via llvm-commits]

probinson 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 | \
----------------
jhenderson wrote:
> 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?
The point is to test that response files work, a feature that is described in the help text.
I can take that out if you think it's unnecessary.


================
Comment at: llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml:53-56
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x1C8
+    Link:            .dynstr
+    AddressAlign:    0x8
----------------
jhenderson wrote:
> 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.
I got to this point by doing obj2yaml on one of the previous .so files and then deleting things that were clearly irrelevant and didn't make the test fail.  I have no idea which fields/sections are necessary and there was a limit to how much fiddling I was willing to do.  If you have guidance in this respect I'd be happy to reduce it further.


================
Comment at: llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml:70-73
+    Type:            STT_FUNC
+    Section:         .text
+    Binding:         STB_GLOBAL
+    Value:           0x3D50
----------------
jhenderson wrote:
> 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!)?
The tool checks Type and Binding.  I have no idea whether the other fields matter; they came with the obj2yaml output.  If you tell me I can delete Section and Value, I'm okay to do that.

I do need all of these symbols, because the object file has to define exactly the same set of symbols that TLI expects to find for this target.


================
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 ";
----------------
jhenderson wrote:
> 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.
These are the mangling prefixes used in the list of functions known to TLI, and the mangled names are all for C++ operators.  If there will be Rust or D-specific library functions added to TLI, then yes this function would need to be updated.  It's not intended to work for arbitrary functions, just the ones that TLI knows about.

The idea was to avoid the relatively expensive demangling and string comparison for names that wouldn't need it; but as the printable name isn't used that much, I can recode this to do the trial demangle and string compare.


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

https://reviews.llvm.org/D114478