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

[liadz0rz]

Summary

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

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

[jhenderson]

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

1642–1643

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

[liadz0rz]

This comment was removed by liadz0rz.

[liadz0rz]

In D72992#1829062, @jhenderson wrote:

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?

[jhenderson]

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

[liadz0rz]

In D72992#1830736, @jhenderson wrote:

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

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?

[jhenderson]

In D72992#1831132, @liadz0rz wrote:

In D72992#1830736, @jhenderson wrote:

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

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.

[liadz0rz]

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

[grimar]

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

[grimar]

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]

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]

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

148

What's full stop?

[grimar]

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

148

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

[jhenderson]

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]

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]

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 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]

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]

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

191

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

[MaskRay]

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]

Update formatting and test names

[liadz0rz]

The previous diff was the wrong one

[jhenderson]

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 commit message

[liadz0rz]

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

[grimar]

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

167

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

215

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

[liadz0rz]

Added RELA and removed trailing newline at end of file

[jhenderson]

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.

215

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

This still hasn't been addressed.

[liadz0rz]

Fixed missing newline and aligned values with spaces

[jhenderson]

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]

Fixed file header

[grimar]

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

184

Why there is no relocation for local here?

[liadz0rz]

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

184

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

[grimar]

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]

Added local test to rela as well

[liadz0rz]

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

184

Fixed :)

[grimar]

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.

[jhenderson]

LGTM.

[liadz0rz]

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

[jhenderson]

In D72992#1853492, @liadz0rz wrote:

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]

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

[liadz0rz]

Fixed failed tests

[jhenderson]

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

[jhenderson]

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

[liadz0rz]

Hey,
For the email please user [e-mail] and for the author please use "Liad Mordekoviz",
Again thank you very much for all the kind help!

[thakis]

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.

[jhenderson]

In D72992#1871939, @thakis wrote:

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

[jhenderson]

In D72992#1872015, @jhenderson wrote:

In D72992#1871939, @thakis wrote:

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.

[rupprecht]

In D72992#1871939, @thakis wrote:

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.

[rupprecht]

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.

llvm/test/tools/llvm-objdump/WebAssembly/relocations.test

-; RUN: llc -mtriple=wasm32-unknown-unknown -filetype=obj %s -o - | llvm-objdump -r - | FileCheck %s
+; RUN: llc -mtriple=wasm32-unknown-unknown -filetype=obj %s -o - | llvm-objdump -r - | FileCheck %s --strict-whitespace --match-full-lines
 
 @foo = external global i32, align 4
 @bar = global i32* @foo, align 4
 
 define void @make_call(void ()* nocapture %func) {
 entry:
   call void %func()
   ret void
 }
 
-; CHECK:      RELOCATION RECORDS FOR [CODE]:
-; CHECK-NEXT: 00000006 R_WASM_TYPE_INDEX_LEB 1+0
+;      CHECK:RELOCATION RECORDS FOR [CODE]:
+; CHECK-NEXT:OFFSET   TYPE                     VALUE
+; CHECK-NEXT:00000006 R_WASM_TYPE_INDEX_LEB    1+0
 
-; CHECK:      RELOCATION RECORDS FOR [DATA]:
-; CHECK-NEXT: 00000006 R_WASM_MEMORY_ADDR_I32 foo+0
+;      CHECK:RELOCATION RECORDS FOR [DATA]:
+; CHECK-NEXT:OFFSET   TYPE                     VALUE
+; CHECK-NEXT:00000006 R_WASM_MEMORY_ADDR_I32   foo+0

llvm/test/tools/llvm-objdump/coff-many-relocs.test

 // Test that llvm-objdump can handle IMAGE_SCN_LNK_NRELOC_OVFL.
 // RUN: llvm-objdump -r %p/Inputs/many-relocs.obj-i386 | FileCheck %s
 
 CHECK:      RELOCATION RECORDS FOR [.text]:
-CHECK-NEXT: IMAGE_REL_I386_DIR16 foo
-CHECK-NEXT: IMAGE_REL_I386_REL16 foo
-CHECK-NEXT: IMAGE_REL_I386_DIR32 foo
-CHECK-NEXT: IMAGE_REL_I386_DIR32NB foo
-CHECK-NEXT: IMAGE_REL_I386_SEG12 foo
-CHECK-NEXT: IMAGE_REL_I386_SECTION foo
-CHECK-NEXT: IMAGE_REL_I386_SECREL foo
-CHECK-NEXT: IMAGE_REL_I386_TOKEN foo
-CHECK-NEXT: IMAGE_REL_I386_SECREL7 foo
-CHECK-NEXT: IMAGE_REL_I386_REL32 foo
+CHECK-NEXT: TYPE                     VALUE
+CHECK-NEXT: IMAGE_REL_I386_DIR16     foo
+CHECK-NEXT: IMAGE_REL_I386_REL16     foo
+CHECK-NEXT: IMAGE_REL_I386_DIR32     foo
+CHECK-NEXT: IMAGE_REL_I386_DIR32NB   foo
+CHECK-NEXT: IMAGE_REL_I386_SEG12     foo
+CHECK-NEXT: IMAGE_REL_I386_SECTION   foo
+CHECK-NEXT: IMAGE_REL_I386_SECREL    foo
+CHECK-NEXT: IMAGE_REL_I386_TOKEN     foo
+CHECK-NEXT: IMAGE_REL_I386_SECREL7   foo
+CHECK-NEXT: IMAGE_REL_I386_REL32     foo

llvm/test/tools/llvm-objdump/hex-relocation-addr.test

 // This test checks that relocation addresses are printed in hex
 // RUN: llvm-objdump -r %p/Inputs/win64-unwind.exe.coff-x86_64.obj | FileCheck %s
 
 CHECK: RELOCATION RECORDS FOR [.pdata]:
-CHECK-NEXT: 0 IMAGE_REL_AMD64_ADDR32NB func
-CHECK-NEXT: 4 IMAGE_REL_AMD64_ADDR32NB func
-CHECK-NEXT: 8 IMAGE_REL_AMD64_ADDR32NB .xdata
-CHECK-NEXT: c IMAGE_REL_AMD64_ADDR32NB func
-CHECK-NEXT: 10 IMAGE_REL_AMD64_ADDR32NB func
-CHECK-NEXT: 14 IMAGE_REL_AMD64_ADDR32NB .xdata
-CHECK-NEXT: 18 IMAGE_REL_AMD64_ADDR32NB smallFunc
-CHECK-NEXT: 1c IMAGE_REL_AMD64_ADDR32NB smallFunc
-CHECK-NEXT: 20 IMAGE_REL_AMD64_ADDR32NB .xdata
-CHECK-NEXT: 24 IMAGE_REL_AMD64_ADDR32NB allocFunc
-CHECK-NEXT: 28 IMAGE_REL_AMD64_ADDR32NB allocFunc
-CHECK-NEXT: 2c IMAGE_REL_AMD64_ADDR32NB .xdata
+CHECK-NEXT: OFFSET           TYPE                     VALUE
+CHECK-NEXT: 0000000000000000 IMAGE_REL_AMD64_ADDR32NB func
+CHECK-NEXT: 0000000000000004 IMAGE_REL_AMD64_ADDR32NB func
+CHECK-NEXT: 0000000000000008 IMAGE_REL_AMD64_ADDR32NB .xdata
+CHECK-NEXT: 000000000000000c IMAGE_REL_AMD64_ADDR32NB func
+CHECK-NEXT: 0000000000000010 IMAGE_REL_AMD64_ADDR32NB func
+CHECK-NEXT: 0000000000000014 IMAGE_REL_AMD64_ADDR32NB .xdata
+CHECK-NEXT: 0000000000000018 IMAGE_REL_AMD64_ADDR32NB smallFunc
+CHECK-NEXT: 000000000000001c IMAGE_REL_AMD64_ADDR32NB smallFunc
+CHECK-NEXT: 0000000000000020 IMAGE_REL_AMD64_ADDR32NB .xdata
+CHECK-NEXT: 0000000000000024 IMAGE_REL_AMD64_ADDR32NB allocFunc
+CHECK-NEXT: 0000000000000028 IMAGE_REL_AMD64_ADDR32NB allocFunc
+CHECK-NEXT: 000000000000002c IMAGE_REL_AMD64_ADDR32NB .xdata
 

llvm/test/tools/llvm-objdump/relocation-xindex-symbol.test

 ## Test that llvm-objdump can handle SHN_XINDEX. The relocation points to a section
 ## symbol that has st_shndx == SHN_XINDEX. To print the section name llvm-objdump
 ## has to use the SHT_SYMTAB_SHNDX section.
 
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-objdump -r %t | FileCheck %s
 
 # CHECK:      RELOCATION RECORDS FOR [.text]:
-# CHECK-NEXT: 0000000000000000 R_X86_64_32 bar
+# CHECK-NEXT: OFFSET           TYPE                     VALUE
+# CHECK-NEXT: 0000000000000000 R_X86_64_32              bar
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:

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

 # RUN: yaml2obj --docnum=1 %s > %t
 # RUN: llvm-objdump --reloc %t > %t1
 # RUN: llvm-objdump -r      %t > %t2
 # RUN: cmp %t1 %t2
-# RUN: FileCheck %s --input-file=%t1
+# RUN: FileCheck %s --input-file=%t1 --strict-whitespace --match-full-lines
 
-# CHECK:      RELOCATION RECORDS FOR [.text]:
-# CHECK-NEXT: 0000000000000001 R_X86_64_32 glob1
-# CHECK-NEXT: 0000000000000001 R_X86_64_32S glob2
-# CHECK-NEXT: 0000000000000002 R_X86_64_64 loc1
-# CHECK-NEXT: 0000000000000001 R_X86_64_32 glob1+0x1
-# CHECK-NEXT: 0000000000000001 R_X86_64_32S glob2+0x2
-# CHECK-NEXT: 0000000000000002 R_X86_64_64 loc1+0x3
+#      CHECK:RELOCATION RECORDS FOR [.text]:
+# CHECK-NEXT:OFFSET           TYPE                     VALUE
+# CHECK-NEXT:0000000000000001 R_X86_64_32              glob1
+# CHECK-NEXT:0000000000000001 R_X86_64_32S             glob2
+# CHECK-NEXT:0000000000000002 R_X86_64_64              loc1
+# CHECK-NEXT:0000000000000001 R_X86_64_32              glob1+0x1
+# CHECK-NEXT:0000000000000001 R_X86_64_32S             glob2+0x2
+# CHECK-NEXT:0000000000000002 R_X86_64_64              loc1+0x3
 
 --- !ELF
 FileHeader: !FileHeader
   Class: ELFCLASS64
   Data: ELFDATA2LSB
   Type: ET_REL
   Machine: EM_X86_64
 
- Offset: 0x4
       Type:   R_X86_64_64
       Addend: 0
 Symbols:
   - Name:    glob
     Section: .text
     Value:   0x0
     Size:    0
     Binding: STB_GLOBAL
+
+## Check relocation formatting on 32 bit as well to verify the formatting is correct.
+# RUN: yaml2obj --docnum=4 %s > %t4
+# RUN: llvm-objdump -r %t4 | FileCheck %s --check-prefix=ELF32 --strict-whitespace --match-full-lines
+
+#      ELF32:RELOCATION RECORDS FOR [.text]:
+# ELF32-NEXT:OFFSET   TYPE                     VALUE
+# ELF32-NEXT:00000001 R_386_NONE               glob1
+# ELF32-NEXT:00000001 R_386_PC32               glob2
+# ELF32-NEXT:00000002 R_386_32                 loc1
+# ELF32-NEXT:00000001 R_386_NONE               glob1+0x1
+# ELF32-NEXT:00000001 R_386_PC32               glob2+0x2
+# ELF32-NEXT:00000002 R_386_32                 loc1+0x3
+
+--- !ELF
+FileHeader: !FileHeader
+  Class: ELFCLASS32
+  Data: ELFDATA2LSB
+  Type: ET_REL
+  Machine: EM_386
+Sections:
+- Name: .text
+  Type: SHT_PROGBITS
+  Content: "0000000000000000"
+  AddressAlign: 8
+  Flags: [SHF_ALLOC]
+- Name: .rel.text
+  Type: SHT_REL
+  Info: .text
+  AddressAlign: 4
+  Relocations:
+    - Offset: 0x1
+      Symbol: glob1
+      Type: R_386_NONE
+    - Offset: 0x1
+      Symbol: glob2
+      Type: R_386_PC32
+    - Offset: 0x2
+      Symbol: loc1
+      Type: R_386_32
+- Name: .rela.text
+  Type: SHT_RELA
+  Link: .symtab
+  Info: .text
+  AddressAlign: 4
+  Relocations:
+    - Offset: 0x1
+      Addend: 1
+      Symbol: glob1
+      Type: R_386_NONE
+    - Offset: 0x1
+      Addend: 2
+      Symbol: glob2
+      Type: R_386_PC32
+    - Offset: 0x2
+      Addend: 3
+      Symbol: loc1
+      Type: R_386_32
+Symbols:
+  - Name: loc1
+  - Name: loc2
+  - Name:    glob1
+    Section: .text
+    Value:   0x0
+    Size:    4
+    Binding: STB_GLOBAL
+  - Name:    glob2
+    Binding: STB_GLOBAL

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

 ## Check that objdump doesn't print relocations in non-relocatable object files.
 
 # RUN: yaml2obj --docnum=1 %s -o %t1
 # RUN: llvm-objdump -r %t1 | FileCheck %s -DFILE=%t1 --check-prefix=FMT --implicit-check-not={{.}}
 # RUN: yaml2obj --docnum=2 %s -o %t2
 # RUN: llvm-objdump -r %t2 | FileCheck %s -DFILE=%t2 --check-prefix=FMT --implicit-check-not={{.}}
 # RUN: yaml2obj --docnum=3 %s -o %t3
 # RUN: llvm-objdump -r %t3 | FileCheck %s -DFILE=%t3 --check-prefixes=FMT,REL --implicit-check-not={{.}}
 
 # FMT: [[FILE]]: file format ELF64-x86-64
 
 # REL:      RELOCATION RECORDS FOR []:
-# REL-NEXT: 0000000000000123 R_X86_64_NONE *ABS*+0x141
+# REL-NEXT: OFFSET           TYPE                     VALUE
+# REL-NEXT: 0000000000000123 R_X86_64_NONE            *ABS*+0x141
 
 ## Executable.
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
   - Name: .rela.plt
     Type: SHT_RELA
     Relocations:

llvm/tools/llvm-objdump/llvm-objdump.cpp

if (!SecOrErr)
                       "): unable to get a relocation target: " +
                       toString(SecOrErr.takeError()));
     SecToRelSec[**SecOrErr].push_back(Section);
   }
 
   for (std::pair<SectionRef, std::vector<SectionRef>> &P : SecToRelSec) {
     StringRef SecName = unwrapOrError(P.first.getName(), Obj->getFileName());
     outs() << "RELOCATION RECORDS FOR [" << SecName << "]:\n";
+    uint32_t OffsetPadding = (Obj->getBytesInAddress() > 4 ? 16 : 8);
+    uint32_t TypePadding = 24;
+    outs() << left_justify("OFFSET", OffsetPadding) << " "
+           << left_justify("TYPE", TypePadding) << " "
+           << "VALUE\n";
 
     for (SectionRef Section : P.second) {
       for (const RelocationRef &Reloc : Section.relocations()) {
         uint64_t Address = Reloc.getOffset();
         SmallString<32> RelocName;
         SmallString<32> ValueStr;
         if (Address < StartAddress || Address > StopAddress || getHidden(Reloc))
           continue;
         Reloc.getTypeName(RelocName);
         if (Error E = getRelocationValueString(Reloc, ValueStr))
           reportError(std::move(E), Obj->getFileName());
 
-        outs() << format(Fmt.data(), Address) << " " << RelocName << " "
-               << ValueStr << "\n";
+        outs() << format(Fmt.data(), Address) << " "
+               << left_justify(RelocName, TypePadding) << " " << ValueStr
+               << "\n";
       }
     }
     outs() << "\n";
   }
 }
 
 void printDynamicRelocations(const ObjectFile *Obj) {
   // For the moment, this option is for ELF only