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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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)); | |
Comment Actions
Thank you for the quick response.
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?
Comment Actions
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.
Comment Actions
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?) | |
Comment Actions
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.
| llvm/tools/llvm-objdump/ELFDump.cpp | ||
|---|---|---|
| 109 | Yes. Is there a better abs implementation or function in LLVM that works around this case? | |
Comment Actions
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. | |
| llvm/tools/llvm-objdump/ELFDump.cpp | ||
|---|---|---|
| 109 | Yes, I need the absolute value for the string representation so foo-0x4 rather than foo+0xfffffffc.... | |
| 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. | |
Comment Actions
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.
| llvm/tools/llvm-objdump/ELFDump.cpp | ||
|---|---|---|
| 109 | Might be tricky given that objtoyaml can't seem to parse large signed integers, | |
Comment Actions
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 | |
Comment Actions
- formatting
| llvm/tools/llvm-objdump/ELFDump.cpp | ||
|---|---|---|
| 109 | Yeah, strange.... maybe I had a typo in my Addend... working now. | |
Comment Actions
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...
| 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) | |
| llvm/tools/llvm-objdump/ELFDump.cpp | ||
|---|---|---|
| 109 | Good to know, thank you! | |
Comment Actions
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. | |
Comment Actions
Splitting/moving the test. I say "major" as in it needs doing or the test will fail on some build bots at least.
Comment Actions
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). | |
Comment Actions
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. | |
Comment Actions
Anyways please feel free to commit it (I did not see James approved it already and it generally looks fine anyways. Thanks!)
| llvm/test/tools/llvm-objdump/relocations-elf.test | ||
|---|---|---|
| 99 | Do any of these CHECK lines need to use ADDEND-NEXT? | |
Comment Actions
Ok. This will probably affect over target tests that aren't run by default. Is the correct process to backout this change, then fix?
Comment Actions
I have a made a change with a fix for this target. Tests pending. Where can I find a list of experimental targets?
Comment Actions
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.
Comment Actions
Thanks, that's what I've just done. Seemingly just ARC and AVR that are experimental.
I have fixed the tests here D70438.
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).