[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 | \
----------------
probinson wrote:
> 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.
Ah, fair enough. Leave it in, but perhaps add a comment to say that that's what this is testing specifically (it looked to me like this was just trying to workaround something by using a response file).


================
Comment at: llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml:53-56
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x1C8
+    Link:            .dynstr
+    AddressAlign:    0x8
----------------
probinson wrote:
> 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.
I'm not familiar with what the tool looks at, so being able to point out which symbols are needed is a little tricky. We've tried to minimise unnecessary obj2yaml output, but it's not always possible to get it to the bare minimum required for a specific piece of code.


================
Comment at: llvm/test/tools/llvm-tli-checker/ps4-tli-check.yaml:70-73
+    Type:            STT_FUNC
+    Section:         .text
+    Binding:         STB_GLOBAL
+    Value:           0x3D50
----------------
probinson wrote:
> 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.
Removing the `Section` field makes the symbols undefined. That may or may not be an issue, but at a guess, you don't want your symbols to be undefined, and such symbols may not want to be considered "in the SDK" for the purposes of the tool, right?


================
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 ";
----------------
probinson wrote:
> 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.
> 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.

I guess the expensive bit is really checking whether the output is the same as the input, since the demangling will fail as fast as those checks would have done. Perhaps we need a new demangle signature that indicates whether demangling has occurred. Probably only worth it if there is a performance concern though.


================
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;
----------------
jhenderson wrote:
> 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.
Usual format is "warning: file name: message" (see what `FileError` does for a comparison).


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

https://reviews.llvm.org/D114478