This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] - Add column headers for relocation printing
ClosedPublic

Authored by liadz0rz on Jan 19 2020, 1:42 AM.

Details

Summary

This allows us better readability and
compatibility with GNU objdump prints.

https://bugs.llvm.org/show_bug.cgi?id=43941

Diff Detail

Event Timeline

liadz0rz created this revision.Jan 19 2020, 1:42 AM

Thanks for the patch!

It looks like you haven't run the tests. Please make sure to run the check-all target, to run all the tests. In particular, I expect to see many tests that will fail, because they do not expect the extra line for the headers. Also, please make sure to update the llvm/test/tools/llvm-objdump/elf-relocations.test to check this works. You will need to add a 32-bit test case probably, to show that the 16 versus 8 space padding decision is made correctly. If you need help writing these tests, please let me know, and I'll be happy to help out.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1648–1649

LLVM's normal variable naming style is UpperCamelCase, so this should be OffsetPadding, and TypePadding.

liadz0rz marked an inline comment as done.Jan 20 2020, 10:36 AM
This comment was removed by liadz0rz.

Thanks for the patch!

It looks like you haven't run the tests. Please make sure to run the check-all target, to run all the tests. In particular, I expect to see many tests that will fail, because they do not expect the extra line for the headers. Also, please make sure to update the llvm/test/tools/llvm-objdump/elf-relocations.test to check this works. You will need to add a 32-bit test case probably, to show that the 16 versus 8 space padding decision is made correctly. If you need help writing these tests, please let me know, and I'll be happy to help out.

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'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?

liadz0rz added a comment.EditedJan 21 2020, 6:17 AM

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?

OK I was mistaken, sorry about that, I've fixed the tests so they all contain the headers, 2 notes on that:

  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?
  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'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.

  1. 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.

liadz0rz updated this revision to Diff 239392.Jan 21 2020, 11:36 AM

Updated with the requested changes and with new tests added and updated

I think you can only add --strict-whitespaces to relocations-elf.test. No need to touch and change another tests probably,
it is enough to test the paddings in one place once and not spread this check everywhere (if there is no valid reason).

llvm/test/tools/llvm-objdump/relocations-elf.test
148

Missing a full stop.

150

I'd 32BITPADDING -> ELF32

153

I think you either need to use --match-full-lines or use {{^}} which checks the begining of the line:

# 32BITPADDING: {{^}}OFFSET   TYPE                     VALUE

because currently you do not check the paddings at the begining of the lines.

167

I see that you've basing on the YAML from above, but we usually do not
leave such empty lines in YAMLs (see another tests), so please remove.

191

It is strange to see SHT_RELA section in a ELFCLASS32 object.
What do you want to check with it? I see it is what the test case above did, but I do not know why it does that :)

211

It seems enough to have one local and one global symbol here.
(the binding does not affect the output anyways it seems).

218

It is generally a good practice to minimize the YAML used.
This one contains excessive parts.
Also, we often format it to improve readability.

I think you can use something like this:

--- !ELF
FileHeader:
  Class:   ELFCLASS32
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_386
Sections:
- Name: .text
  Type: SHT_PROGBITS
- Name: .rel.text
  Type: SHT_REL
  Info: .text
  Relocations:
    - Offset: 0x1
      Symbol: global
      Type:   R_386_PC32
    - Offset: 0x2
      Symbol: loc
      Type:   R_386_32
Symbols:
  - Name:    loc
  - Name:    global
    Binding: STB_GLOBAL

Looking again, it seems it is OK to keep --strict-whitespace in WebAssembly/relocations.test as it is a generic test for relocations output for WASM.
I do not think we should add --strict-whitespace to COFF tests here as they are testing different things, not just regular output.
(i.e. it is probably not a proper place, they might want to add a generic test probably).
I wonder what other people thing about this though.

grimar added inline comments.Jan 22 2020, 12:57 AM
llvm/test/tools/llvm-objdump/relocations-elf.test
191

(I mean it has SHT_REL, what is unnatural for ELFCLASS64 in the same way like SHT_RELA is unnatural for ELFCLASS32).

liadz0rz marked an inline comment as done.Jan 22 2020, 11:37 PM
liadz0rz added inline comments.
llvm/test/tools/llvm-objdump/relocations-elf.test
148

What's full stop?

grimar added inline comments.Jan 23 2020, 12:30 AM
llvm/test/tools/llvm-objdump/relocations-elf.test
148

The symbol . used in writing at the end of a sentence.

jhenderson added inline comments.Jan 23 2020, 8:13 AM
llvm/test/tools/llvm-objdump/relocations-elf.test
148

I'd change "padding" to "formatting" as it also shows that the offset fieds are of the appropriate size.

191

I'd keep both SHT_REL and SHT_RELA sections. The gABI doesn't limit these sections to architectures of one or other size (although some processor supplements do), so we should show we can handle all situations. I know at least one system I worked on used SHT_REL originally and then later swapped to SHT_RELA.

llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
9

I'm not sure you need this additional test case here. I don't think it adds anything above the line above.

liadz0rz marked 2 inline comments as done.Jan 23 2020, 8:37 AM
liadz0rz added inline comments.
llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
9

Checking strict-whitespaces on FMT kept failing and it didn't really matter on the padding change

jhenderson added inline comments.Jan 24 2020, 1:07 AM
llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
9

Okay. Regardless, there's no need for a --strict-whitespace check here, as @grimar mentioned. We only need --strict-whitespace for the "main" relocations test for a given format (i.e. relocations-elf.test).

liadz0rz updated this revision to Diff 240386.Jan 25 2020, 9:22 AM

Updated required changes, removed unnecessary additions of --strict-whitespace, I did not change the content of the ELF32 test because there was no conclusive decision to whether I should or shouldn't remove the extra relocations on that test.

jhenderson added inline comments.Jan 27 2020, 1:38 AM
llvm/test/tools/llvm-objdump/WebAssembly/relocations.test
12–14

As a slight readability suggestion, consider doing the following:

; CHECK:RELOCATION RECORDS FOR [CODE]:

This allows for the ':' characters and therefore the actual checked-for text to line up.

Same applies a few lines below.

llvm/test/tools/llvm-objdump/relocations-elf.test
7–14

Same comment as the WASM test.

150

ELF32 is the canonical naming style for 32-bit ELF (and ELF64 for 64-bit ELF). Please use that, not 32ELF, as @grimar originally asked for.

152

Same comment as other tests.

191

@grimar, are you happy with my suggestion of keeping both?

grimar added inline comments.Jan 27 2020, 1:43 AM
llvm/test/tools/llvm-objdump/relocations-elf.test
191

Ah, yes, sorry for not adding an explicit comment about it.

and allows for better

Looks like the summary misses some sentence.

llvm/test/tools/llvm-objdump/relocations-elf.test
191

lld accepts both SHT_REL and SHT_RELA input. It produces either SHT_REL or SHT_RELA depending on config->isRela (like a target preference).

powerpc and riscv32 are two 32-bit targets using RELA.

liadz0rz marked 13 inline comments as done.Jan 29 2020, 2:51 AM
liadz0rz updated this revision to Diff 241083.Jan 29 2020, 2:55 AM

Update formatting and test names

liadz0rz updated this revision to Diff 241086.Jan 29 2020, 3:00 AM

The previous diff was the wrong one

Looks like all my comments have been addressed, but a couple of @grimar's are still outstanding.

llvm/test/tools/llvm-objdump/relocations-elf.test
211

I think you missed this comment?

218

I think you msised this?

liadz0rz updated this revision to Diff 241088.Jan 29 2020, 3:04 AM

Updated commit message

liadz0rz updated this revision to Diff 241089.Jan 29 2020, 3:10 AM
liadz0rz marked 9 inline comments as done.

Fixed commit message yet again and minimized the YAML for the ELF32 test

liadz0rz edited the summary of this revision. (Show Details)Jan 29 2020, 3:11 AM
grimar added inline comments.Jan 29 2020, 3:16 AM
llvm/test/tools/llvm-objdump/relocations-elf.test
167

I think previously we agreed to check both SHT_REL and SHT_RELA here.

194

You need to add a new line to fix "No newline at end of file" warning.

liadz0rz updated this revision to Diff 241103.Jan 29 2020, 3:43 AM
liadz0rz marked 2 inline comments as done.

Added RELA and removed trailing newline at end of file

jhenderson added inline comments.Jan 30 2020, 1:19 AM
llvm/test/tools/llvm-objdump/relocations-elf.test
159

Delete "!FileHeader". It serves no purpose. You might want to do it for the other doc in this file in a separate change too.

173

Nit: Add some spaces to make the values all line up, i.e.

- Offset: 0x1
  Symbol: global
  Type:   R_386_32

Same goes below for the other relocations.

194

You need to add a new line to fix "No newline at end of file" warning.

This still hasn't been addressed.

liadz0rz updated this revision to Diff 241441.Jan 30 2020, 6:45 AM
liadz0rz marked 3 inline comments as done.

Fixed missing newline and aligned values with spaces

jhenderson added inline comments.Jan 30 2020, 6:56 AM
llvm/test/tools/llvm-objdump/relocations-elf.test
159

You've deleted too much. I only wanted the bit after the ':' deleted. Please reinstate the FileHeader: part.

Does this work in its current state? That surprises me if it does.

llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test
18

Here's an example of what a FileHeader declaration should look like.

liadz0rz updated this revision to Diff 241531.Jan 30 2020, 11:37 AM
liadz0rz marked 2 inline comments as done.

Fixed file header

grimar added inline comments.Jan 31 2020, 12:37 AM
llvm/test/tools/llvm-objdump/relocations-elf.test
184

Why there is no relocation for local here?

liadz0rz marked 2 inline comments as done.Jan 31 2020, 1:27 AM
liadz0rz added inline comments.
llvm/test/tools/llvm-objdump/relocations-elf.test
184

Since you asked for a minimal YAML, I don't mind adding one.

grimar added inline comments.Jan 31 2020, 3:27 AM
llvm/test/tools/llvm-objdump/relocations-elf.test
184

The YAML description I've suggested earlier tested both local and global:

Relocations:
  - Offset: 0x1
    Symbol: global
    Type:   R_386_PC32
  - Offset: 0x2
    Symbol: loc
    Type:   R_386_32

Also, I see no reason to test both local and global symbol for SHT_RELA/SHT_REL in the ELFCLASS64 test,
test both for SHT_REL in the ELFCLASS32 test, but omit the local symbol case for the SHT_RELA.
We want to be consistent usually.

So yes, while we test both local and global everywhere, this place should not be an exception I think.

liadz0rz updated this revision to Diff 241689.Jan 31 2020, 4:09 AM
liadz0rz marked an inline comment as done.

Added local test to rela as well

liadz0rz marked 2 inline comments as done.Jan 31 2020, 4:10 AM
liadz0rz added inline comments.
llvm/test/tools/llvm-objdump/relocations-elf.test
184

Fixed :)

I have no more comments except the single nit.
Perhaps @jhenderson should give a final approve for this.

llvm/test/tools/llvm-objdump/relocations-elf.test
157

I'd use R_386_PC32 here to be consistent with local above.
The consistency is important for readability.

This revision is now accepted and ready to land.Jan 31 2020, 4:37 AM
liadz0rz marked an inline comment as done.Feb 2 2020, 1:10 AM

Thank you all for your help with this patch, I do not have permissions to merge, how can I get this merged?

Thank you all for your help with this patch, I do not have permissions to merge, how can I get this merged?

I'll take a look at committing this one for you. What email address and author values do you want me to include for your git commit message?

Once this has landed, you may want to consider requesting commit access (https://llvm.org/docs/DeveloperPolicy.html#new-contributors).

jhenderson requested changes to this revision.Feb 3 2020, 3:54 AM

Hi @liadz0rz,

I tried applying this locally and there were a number of test failures. Could you please make sure to run check-llvm to run all the tests again and fix any failures. In particular, the following tests all failed for me, with what look to be issues related to your patch:

LLVM :: MC/ARM/dwarf-asm-multiple-sections-dwarf-2.s
LLVM :: MC/ARM/dwarf-asm-multiple-sections.s
LLVM :: MC/ARM/dwarf-asm-nonstandard-section.s
LLVM :: MC/ARM/dwarf-asm-single-section.s
LLVM :: MC/COFF/cfi-sections.s
LLVM :: Object/objdump-relocations.test
This revision now requires changes to proceed.Feb 3 2020, 3:54 AM
liadz0rz updated this revision to Diff 243755.Feb 11 2020, 12:58 AM

Fixed failed tests

jhenderson accepted this revision.Feb 11 2020, 1:37 AM

LGTM, I'll have another stab at landing this.

This revision is now accepted and ready to land.Feb 11 2020, 1:37 AM

@liadz0rz, I've checked and this change passes the tests locally, but you didn't answer this comment:

What email address and author values do you want me to include for your git commit message?

Please let me know, so that I can commit this for you.

Hey,
For the email please user liad.mordekoviz@gmail.com and for the author please use "Liad Mordekoviz",
Again thank you very much for all the kind help!

MaskRay accepted this revision.Feb 11 2020, 2:10 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Feb 12 2020, 4:45 AM

This broke tests everywhere when the experimental AVR backend is enabled, e.g. http://45.33.8.238/

Please take a look, and if it takes a while to fix please revert while you investigate.

This broke tests everywhere when the experimental AVR backend is enabled, e.g. http://45.33.8.238/

Please take a look, and if it takes a while to fix please revert while you investigate.

I have something else going on on my machine at the moment, so this will be tricky for me to sort out. If somebody else could fix it, that would be great. The actual fix should be trivial. In llvm/test/MC/AVR/symbol_relocation.s, add the line ; CHECK-NEXT: OFFSET TYPE VALUE immediately after the CHECK for "RELOCATION RECORDS".

This broke tests everywhere when the experimental AVR backend is enabled, e.g. http://45.33.8.238/

Please take a look, and if it takes a while to fix please revert while you investigate.

I have something else going on on my machine at the moment, so this will be tricky for me to sort out. If somebody else could fix it, that would be great. The actual fix should be trivial. In llvm/test/MC/AVR/symbol_relocation.s, add the line ; CHECK-NEXT: OFFSET TYPE VALUE immediately after the CHECK for "RELOCATION RECORDS".

I finished what I was doing and submitted a quick fix. I haven't got the time to enable an experimental target on my machine, so the fix is technically speculative, but I'm confident in it. If there are more failures of the same kind, please consider fixing them directly rather than asking me to fix them.

This broke tests everywhere when the experimental AVR backend is enabled, e.g. http://45.33.8.238/

Please take a look, and if it takes a while to fix please revert while you investigate.

Is it actually the llvm policy that build breakages/failures for experimental backends means reverting culprit patches? I thought that one of the things "experimental" means is that the build bot breakages specific to those backends are addressed by the owners of said experimental backend.

Oh, I missed that James already posted to llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2020-February/139115.html. Please reply there instead.