This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] Prefix memory operand addresses with '0x'
ClosedPublic

Authored by ikudrin on Jun 25 2021, 5:22 AM.

Details

Summary

This helps to avoid ambiguity when the address contains only digits 0..9.

Diff Detail

Event Timeline

ikudrin created this revision.Jun 25 2021, 5:22 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ikudrin requested review of this revision.Jun 25 2021, 5:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2021, 5:22 AM
jhenderson accepted this revision.Jun 25 2021, 5:26 AM

No objections from me. Best give others a chance to comment though.

This revision is now accepted and ready to land.Jun 25 2021, 5:26 AM

I like the 0x, but the addresses at the start of the lines get printed without it so it introduces an inconsistency. I personally would prefer it if either both used 0x, or both leave out 0x. Is it possible to change both or does that introduce compatibility issues?

I like the 0x, but the addresses at the start of the lines get printed without it so it introduces an inconsistency. I personally would prefer it if either both used 0x, or both leave out 0x. Is it possible to change both or does that introduce compatibility issues?

In fact, this patch fixes some inconsistencies in printing instructions and their comments. Hexadecimal constants are already prefixed with 0x (see, for example, lld/test/ELF/x86-64-gotpc-relax-nopic.s), as well as targets of branch instructions (lld/test/ELF/symver.s), etc.

As for addresses of the instructions, personally, I do not believe they need to be prefixed. They are printed as a sequence, so there are for sure some "letter" digits among them, and thus the actual format is easily recognized. In any case, I guess that that is something unrelated to this patch, and should be done separately, if decided.

In fact, this patch fixes some inconsistencies in printing instructions and their comments. Hexadecimal constants are already prefixed with 0x (see, for example, lld/test/ELF/x86-64-gotpc-relax-nopic.s), as well as targets of branch instructions (lld/test/ELF/symver.s), etc.

As for addresses of the instructions, personally, I do not believe they need to be prefixed. They are printed as a sequence, so there are for sure some "letter" digits among them, and thus the actual format is easily recognized. In any case, I guess that that is something unrelated to this patch, and should be done separately, if decided.

They are closer in meaning to the addresses at the start of the line, not just instructions but data as well, than to anything else though. When we want to see what e.g. pushq 8346(%rip) # 203280 actually refers to, we look for the line that starts with 203280. Yes, I have actually looked things up this way, this isn't hypothetical.

They are closer in meaning to the addresses at the start of the line, not just instructions but data as well, than to anything else though. When we want to see what e.g. pushq 8346(%rip) # 203280 actually refers to, we look for the line that starts with 203280.

Thanks. pushq 8346(%rip) # 203280 is the perfect example of why this patch is useful. The offset here is decimal while the calculated address is hexadecimal, but that is not obvious.

Yes, I have actually looked things up this way, this isn't hypothetical.

Well, in that case, would you like to prepare the corresponding patch? You seem to be able to substantiate it. Note that the disassembler already prints target addresses of jump/branch instructions with the '0x' prefix.

MaskRay accepted this revision.Jun 26 2021, 10:33 AM

LGTM.

int3 accepted this revision.Jun 26 2021, 2:44 PM

I like this :)

This revision was automatically updated to reflect the committed changes.