[PATCH] D72992: [llvm-objdump] - Add column headers for relocation printing

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D72992#1831132 <https://reviews.llvm.org/D72992#1831132>, @liadz0rz wrote:

> In D72992#1830736 <https://reviews.llvm.org/D72992#1830736>, @jhenderson wrote:
>
> > In D72992#1830094 <https://reviews.llvm.org/D72992#1830094>, @liadz0rz wrote:
> >
> > > I've updated the relocations-elf.test file with the new headers, they seem to recognize the headers quite well, however, the relocations on my machine come out different than the test expects, so the other parts of the test fail...
> > >  Are there any requirements for the machine I am running my test on?
> >
> >
> > I'm not aware of any specific requirements for this test. How are you running the test? Have you remembered to build the other tools in LLVM that it uses? (yaml2obj, llvm-objdump, FileCheck?). What is the output you get when you run the test?
>
>
> I ran the tests in 2 ways:
>
> OK I was mistaken, sorry about that, I've fixed the tests so they all contain the headers, 2 notes on that:


Please could you upload the latest patch including your test changes, so that I can review them.

> 1. it seems that the tests do not really test white spaces for some reason (I tried randomly changing the padding in the test to check that, as long as the right words were written in the right order all was well) does it make any sense to you?

By default, FileCheck ignores leading and trailing whitespace on the check pattern's line and canonicalises all other whitespace to a single space. Additionally, it only does partial matches by default (i.e. if the CHECK string is "foo bar", it will succeed if passed "this string contains 'foo bar'"). In most cases, whitespace isn't really important. However, in some cases, like this change, we want to test that the formatting is correct. This is important because some people parse the output and expect exact numbers of spaces, and also, it ensures the output is more readable. In cases where whitespace is important, you can use FileCheck's `--strict-whitespace` option, which doesn't do the whitespace canonicalisation, although it does still strip trailing and leading spaces from the line. That's probably sufficient for your use-case here. (For reference, you can use --match-full-lines to match the entire line, rather than a partial match; when combined with --strict-whitespace, it also makes sure leading and trailing whitsepace matches). It might be helpful to refer to the FileCheck documentation <https://llvm.org/docs/CommandGuide/FileCheck.html>.

> 2. The web assembly test already contains a 32-bit address, would you like me to add a specific test case for explicit 32-bit for this matter?

I think adding an ELF 32-bit case would be useful. I'd add it to the same file as the existing ELF 64 case. However, I don't think it is strictly needed, so don't feel obliged to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72992