Page MenuHomePhabricator

[llvm-objdump] Print relocation addends in hexadecimal
ClosedPublic

Authored by davidb on Nov 8 2019, 2:09 AM.

Details

Summary

Matches GNU objdump. Makes debugging easier for me as I'm working out addresses from symbol+addend, so it would be good to be calculating in a single format.

Diff Detail

Event Timeline

davidb created this revision.Nov 8 2019, 2:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2019, 2:09 AM
grimar added a comment.EditedNov 8 2019, 2:23 AM

Seems that printing relocations in hex is reasonable (though I'd like to hear from others too).

You need to change the title to include a tool name. Look at the recent commits for llvm-objdump.
Did you run LLD tests (I'd expect them to fail)?

llvm/tools/llvm-objdump/ELFDump.cpp
111

Will the following work?

if (Addend != 0)
  Fmt << Sign << format("0x%" PRIx64, abs(Addend));
davidb added a comment.Nov 8 2019, 2:26 AM

Thank you for the quick response.

Did you run LLD tests (I'd expect them to fail)?

I just noticed that I haven't and am in the process of building/testing them too. Are there any other projects that might use llvm-objdump for testing?

davidb retitled this revision from Print relocation addends in hexadecimal to [llvm-objdump] Print relocation addends in hexadecimal.Nov 8 2019, 2:28 AM
grimar added a comment.Nov 8 2019, 2:29 AM

Thank you for the quick response.

Did you run LLD tests (I'd expect them to fail)?

I just noticed that I haven't and am in the process of building/testing them too. Are there any other projects that might use llvm-objdump for testing?

I guess so. I'd try to search for "llvm-objdump" invocations to find them out. Probably most of them are covered by check-llvm though.

Thanks for the patch. I'm surprised this hasn't broken any LLD tests. Did you run those too?

GNU objdump also pads with leading zeroes. As the general aim of the tools is to be more compatible, and whilst you're working on this bit, it would make sense to do the same.

llvm/tools/llvm-objdump/ELFDump.cpp
109

Won't this fail for minimum int64_t?

110

const char rather than char const is LLVM style, I believe. (Does clang-format fix this?)

Thank you for the quick response.

Did you run LLD tests (I'd expect them to fail)?

I just noticed that I haven't and am in the process of building/testing them too. Are there any other projects that might use llvm-objdump for testing?

I guess so. I'd try to search for "llvm-objdump" invocations to find them out. Probably most of them are covered by check-llvm though.

I'd check the clang tests, just in case. A search is probably sufficient here.

davidb added a comment.Nov 8 2019, 3:07 AM

Thanks for the patch. I'm surprised this hasn't broken any LLD tests. Did you run those too?

Surprisingly this doesn't break clang/LLD tests... I couldn't find any tests that used objdump -r

GNU objdump also pads with leading zeroes. As the general aim of the tools is to be more compatible, and whilst you're working on this bit, it would make sense to do the same.

So getRelocationValueString is used for the value string when using -d -r and -r

The former isn't padded with leading zeros, and the latter is.

Example:

-d -r
0000000000000000 <bar>:
   0:	55                   	push   %rbp
   1:	48 89 e5             	mov    %rsp,%rbp
   4:	b0 00                	mov    $0x0,%al
   6:	e8 00 00 00 00       	callq  b <bar+0xb>
			7: R_X86_64_PC32	foo-0x4
-r
test.o:     file format elf64-x86-64

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE
0000000000000007 R_X86_64_PC32     foo-0x0000000000000004

So I'd like to update the diff where only zero pad in the case of -r.

davidb marked an inline comment as done.Nov 8 2019, 3:08 AM
davidb added inline comments.
llvm/tools/llvm-objdump/ELFDump.cpp
109

Yes. Is there a better abs implementation or function in LLVM that works around this case?

So I'd like to update the diff where only zero pad in the case of -r.

Sounds reasonable to me.

llvm/tools/llvm-objdump/ELFDump.cpp
109

I'm not aware of one, unfortunately, but do you need it?

My reading of the code is that format(AddendFmt, Addend) combined with the PRIx64 (which uses a signed integer type) you used above should be sufficient.

davidb marked an inline comment as done.Nov 8 2019, 5:53 AM
davidb added inline comments.
llvm/tools/llvm-objdump/ELFDump.cpp
109

Yes, I need the absolute value for the string representation so foo-0x4 rather than foo+0xfffffffc....

jhenderson added inline comments.Nov 8 2019, 6:22 AM
llvm/tools/llvm-objdump/ELFDump.cpp
109

Right, of course, sorry.

The answer to this stack overflow post suggests the way to do it is to cast the type to the unsigned equivalent and then negating it (rather than the other way around). I haven't tested that this works though. You should add a test case for it.

check-lld passes. It is not too surprising - lld tests use llvm-objdump -d but not its other features (I've migrated a lot of llvm-objdump tests to llvm-nm or llvm-readelf where appropriate).

To llvm-objdump contributors: there is a similar feature that prints immediates in hexidecimal: --print-imm-hex. I wonder if we can turn on it by default.

davidb marked an inline comment as done.Nov 12 2019, 5:53 AM
davidb added inline comments.
llvm/tools/llvm-objdump/ELFDump.cpp
109

Might be tricky given that objtoyaml can't seem to parse large signed integers,

By the way, this needs to print differently for 64 bit versus 32 bit ELF, although I'm not sure that this needs to necessarily be part of this change.

llvm/tools/llvm-objdump/ELFDump.cpp
109

yaml2obj can handle the following yaml fine for me (the integer is 0x8000000000000000):

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_X86_64
Sections:
  - Name:    .foo
    Type:    SHT_PROGBITS
  - Name:    .rela.foo
    Type:    SHT_RELA
    Info:    .foo
    Relocations:
      - Offset: 0
        Symbol: foo
        Type: R_X86_64_NONE
        Addend: -9223372036854775808
Symbols:
  - Name: foo
davidb updated this revision to Diff 228887.Nov 12 2019, 7:04 AM
  • Add tets for 64-bit limits checking
davidb updated this revision to Diff 228888.Nov 12 2019, 7:06 AM
davidb marked 2 inline comments as done.
  • formatting
llvm/tools/llvm-objdump/ELFDump.cpp
109

Yeah, strange.... maybe I had a typo in my Addend... working now.

I haven't conditionally zero extended it in the case of -r. I'm not sure consistency with gnu objdump is worth it in that case and as there is a difference with -r -d it makes the code look. bit ugly. But I'm open to change that if you really insist...

MaskRay added inline comments.Nov 12 2019, 10:03 AM
llvm/tools/llvm-objdump/ELFDump.cpp
109

I think std::abs is very inconvenient here.

Probably just use Addend < 0 ? -(uint64_t)Addend : (uint64_t)Addend

(uint64_t)INT64_MIN is implementation-defined before C++20 and defined since C++20. "implementation-defined" does not matter, because there is so much code that relies on similar implementation defined behaviors.

110

You may even delete the variable and inline it as (Addend < 0 ? '-' : '+') (prefer a character to a string)

davidb updated this revision to Diff 229052.Nov 13 2019, 3:04 AM
  • review comments
davidb marked 5 inline comments as done.Nov 13 2019, 3:05 AM
davidb added inline comments.
llvm/tools/llvm-objdump/ELFDump.cpp
109

Good to know, thank you!

Thanks for the update. I've got one major request and a number of cosmetic requests in the test, but otherwise looks good.

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

This test needs moving into the X86 directory, as the disassembler is target dependent. Alternatively, split the inline relocations part into a separate test in that directory.

94

Nit: missing full stop.

108

Delete "!FileHeader"

109–112

Please line up the values so that they are aligned vertically:

Class:   ELFCLASS64
Data:    ELFDATA2LSB
Type:    ET_REL
Machine: EM_X86_64
117

Size: 8 instead of Content would be slightly more readable, I guess.

118

This line can be deleted.

120

This line should be deleted.

124

This line can be deleted.

129

Here and below, add some spaces after "Type:" to make R_X86... line up with the other values.

146–147

Get rid of the extra whitespace here.

149

Add some padding before "glob" to line this up with the rest of the values in the following lines.

davidb marked an inline comment as done.Nov 13 2019, 6:49 AM

What is the major request?

What is the major request?

Splitting/moving the test. I say "major" as in it needs doing or the test will fail on some build bots at least.

davidb updated this revision to Diff 229123.Nov 13 2019, 9:45 AM
  • Split disassemble test case. Fix formatting
  • formatting
jhenderson accepted this revision.Nov 14 2019, 1:31 AM

LGTM, with one nit.

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

Nit: the norm is no whitespace between parts of YAML (i.e. please delete this line).

This revision is now accepted and ready to land.Nov 14 2019, 1:31 AM
davidb updated this revision to Diff 229251.Nov 14 2019, 2:16 AM
  • remove whitesspace

A few minor requests from me.

llvm/test/tools/llvm-objdump/X86/elf-disassemble-relocs.test
88

Please remove this empty line and the one below (we don't have such lines in our another tests and while I think I understand why you added them, it is still inconsistent, what is probably much more important. Another argument is that obj2yaml does not do that too).

100

Could you please comment this and above (or just all the) Addends? They are probably not obvious for a new readers.

118

Do you need 'Size`? I haven't check but I suspect that you dont need 'Size' and 'Binding', I am not sure though.

Anyways please feel free to commit it (I did not see James approved it already and it generally looks fine anyways. Thanks!)

davidb updated this revision to Diff 229303.Nov 14 2019, 6:48 AM
  • Merge branch 'master' into disassembly_hex
  • Fix test after branch update
MaskRay added inline comments.Nov 14 2019, 10:36 AM
llvm/test/tools/llvm-objdump/relocations-elf.test
99

Do any of these CHECK lines need to use ADDEND-NEXT?

davidb updated this revision to Diff 229517.Nov 15 2019, 5:22 AM
  • Merge branch 'master' into disassembly_hex
This revision was automatically updated to reflect the committed changes.

This breaks oodles of AVR tests: http://45.33.8.238/linux/4114/step_10.txt

Ok. This will probably affect over target tests that aren't run by default. Is the correct process to backout this change, then fix?

This breaks oodles of AVR tests: http://45.33.8.238/linux/4114/step_10.txt

I have a made a change with a fix for this target. Tests pending. Where can I find a list of experimental targets?

I don't know if there's a list anywhere. ls llvm/lib/Target shows you all in tree targets, LLVM_ALL_TARGETS in cmake (see llvm-cs.pcc.me.uk) shows the on by default targets, so adding the difference to LLVM_EXPERIMENTAL_TARGETS_TO_BUILD will likely give you all targets.

I don't know if there's a list anywhere. ls llvm/lib/Target shows you all in tree targets, LLVM_ALL_TARGETS in cmake (see llvm-cs.pcc.me.uk) shows the on by default targets, so adding the difference to LLVM_EXPERIMENTAL_TARGETS_TO_BUILD will likely give you all targets.

Thanks, that's what I've just done. Seemingly just ARC and AVR that are experimental.

I have fixed the tests here D70438.