[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:53-56
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x1C8
+    Link:            .dynstr
+    AddressAlign:    0x8
----------------
probinson wrote:
> jhenderson wrote:
> > 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.
> I was actually asking about fields, not symbols, but I think it's as minimal as it can get at this point.
Yes, looks pretty minimal now, since we need lots of symbols.


================
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:
> > 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.
FYI, undefined dynamic symbols are imports.


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:238
+    WithColor::warning() << StringRef(Filepath)
+                         << ": not an Archive or ObjectFile\n";
     return;
----------------
I'd be tempted to use plain English here, rather than class names to describe this situation, as end users don't know about the classes, i.e. "not an archive or object file"


================
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;
----------------
probinson wrote:
> jhenderson wrote:
> > 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).
> Updated to put filename first, here and elsewhere.
Is this warning message tested anywhere? I don't see it immediately, and I'd expect test failures if it were tested elsewhere due to the format change. Same goes for the other warnings/errors reported in this file.


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

https://reviews.llvm.org/D114478