Page MenuHomePhabricator

[llvm-objdump] Fix incomplete relocation output for -D -r mode
ClosedPublic

Authored by jasonliu on Apr 6 2020, 12:44 PM.

Details

Summary

This patch intends to fix incomplete relocation printing for XCOFF (potentially for other targets).

Diff Detail

Event Timeline

jasonliu created this revision.Apr 6 2020, 12:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 12:44 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1571

I think this comment is out of date. There might be others in this block.

1585

I think XCOFF32 is working because the pointer length is the same as the instruction length?

jasonliu updated this revision to Diff 255676.Apr 7 2020, 7:49 AM

Fix comments.

jasonliu marked an inline comment as done.Apr 7 2020, 8:00 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1585

This was mainly a comment about how the output would be displayed. The status quo (relocation prints "in the middle") felt weird to me, but I have now found that it matches the binutils objdump output.

00000000000000c8 <main>:
  c8:   00 00 00 00     .long 0x0
                        c8: R_POS_64    .main
  cc:   00 00 00 00     .long 0x0

Should we add a comment that this checks only the start of the relocation (and not its end) on purpose?

jasonliu updated this revision to Diff 255735.Apr 7 2020, 11:04 AM

Add comments for print in the middle behavior.

jasonliu marked 2 inline comments as done.Apr 7 2020, 11:05 AM
jasonliu added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
1585

Thanks. Comment added.

jasonliu marked an inline comment as done.Apr 7 2020, 11:34 AM
MaskRay added inline comments.Apr 7 2020, 12:10 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1585

This was mainly a comment about how the output would be displayed. The status quo (relocation prints "in the middle") felt weird to me, but I have now found that it matches the binutils objdump output.

I think the idea is that the relocations that may modify the instruction are printed after the instruction.

llvm/test/tools/llvm-objdump/XCOFF/disassemble-all.test
1

I would suggest adding an x86_64 test file (llvm/test/tools/llvm-objdump/MachO/disassemble-relocs-data-x86_64.test) for another object file format that can expose similar issues.

The following, compiled for x86_64-apple-darwin would seem to fit:

extern char s[];
void *p = s + 0x60606060606060;

yaml2obj/obj2yaml does not seem to preserve relocation information for Mach-O, but the output seems to be good enough for the test.

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

Split the sentence using a period instead of using ", and".
s/relocation prints/the relocation print/;

1583

s/. But this matches/, but this is what matches/;

jhenderson added inline comments.Apr 8 2020, 12:47 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1583

Actually, I think "but this matches" is a better phrasing (but should be part of the sentence before as you suggest). I'd also use a colon rather than a full stop for the end of the "Note..." bit. I'd recommend the following, with some other minor rewording too:

"... is not the complete data: we might see the relocation printed in the middle of the data, but this matches the binutils objdump output."

jasonliu updated this revision to Diff 256086.Apr 8 2020, 12:05 PM

Added test case for Macho. Adjusted comments as suggested.

jasonliu marked 4 inline comments as done.Apr 8 2020, 12:06 PM

This LGTM. Thanks.

This revision is now accepted and ready to land.Apr 8 2020, 6:28 PM
MaskRay added inline comments.Apr 9 2020, 12:01 AM
llvm/test/tools/llvm-objdump/MachO/Inputs/relocs-data-macho-x86_64.yaml
3 ↗(On Diff #256086)

The existing problem with obj2yaml's Mach-O port is that the output is too verbose.

Can you check whether some fields can be deleted and reduce the YAML as much as possible?

jhenderson added inline comments.Apr 9 2020, 1:10 AM
llvm/test/tools/llvm-objdump/MachO/Inputs/relocs-data-macho-x86_64.yaml
1 ↗(On Diff #256086)

Is this file used in only one test? If so, it could just be folded into that test.

jasonliu updated this revision to Diff 256294.Apr 9 2020, 7:17 AM

Fold test case into a single file.

@MaskRay
I was trying to remove some fields, but I will get " truncated or malformed object" error from "llvm-objdump -D -r" command for those objects.
I suspect after removing some fields, I will need to edit other fields in order for every offset/length to add up correctly.
So I kept the yaml file as it is.

This revision was automatically updated to reflect the committed changes.

As written, this test writes to the current directory, which assumes that the current directory is writable, but that is not true in all builds. I have committed 215e6bfcfb5af7713ec348f679c7be4d2f32dc82 to fix.

MaskRay added inline comments.Apr 13 2020, 2:21 PM
llvm/test/tools/llvm-objdump/MachO/disassemble-relocs-data-x86_64.test
2

This should be yaml2obj %s -o %.o

(yaml2obj does not record the source filename in the output, which may make FileCheck tests brittle sometimes.)

jasonliu marked an inline comment as done.Apr 13 2020, 2:46 PM

As written, this test writes to the current directory, which assumes that the current directory is writable, but that is not true in all builds. I have committed 215e6bfcfb5af7713ec348f679c7be4d2f32dc82 to fix.

@saugustine Thanks for the quick fix.

llvm/test/tools/llvm-objdump/MachO/disassemble-relocs-data-x86_64.test
2

Thanks for pointing that out. Addressed in post commit: rGd5143e3f102a91599682fc630205e5c6fec58c76.