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

[Paul Robinson via Phabricator via llvm-commits]

probinson marked 3 inline comments as done.
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:
> 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).
Added a comment here, and a few other places to better describe the testing.


================
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:
> 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.


================
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:
> 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.


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

https://reviews.llvm.org/D114478