[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:70-73
+    Type:            STT_FUNC
+    Section:         .text
+    Binding:         STB_GLOBAL
+    Value:           0x3D50
----------------
jhenderson wrote:
> 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?
It never occurred to me that Dynamic Symbols could be undefined, but no I suppose they shouldn't be considered as "in the SDK."  I'll have to do something about that.


================
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:
> 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.
This is the simplest of several demangling APIs.  A few hundred string compares probably aren't a performance concern though, in this context.


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

https://reviews.llvm.org/D114478