Page MenuHomePhabricator

[MC] Add MCInstrAnalysis::evaluateMemoryOperandAddress
ClosedPublic

Authored by seiya on Jun 26 2019, 3:32 PM.

Details

Summary

Add a new method which tries to compute the target address referenced by an operand.

This patch supports x86_64 RIP-relative addressing for now.

It is necessary to print referenced symbol names in llvm-objdump.

Diff Detail

Repository
rL LLVM

Event Timeline

seiya created this revision.Jun 26 2019, 3:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 3:32 PM

Looks like you need to clang-format your changes?

bcain added a subscriber: bcain.Jun 27 2019, 8:53 AM
bcain added inline comments.
llvm/include/llvm/MC/MCInstrAnalysis.h
157 ↗(On Diff #206755)

It may make more sense for this to return an llvm::Optional<uint64_t>.

Are you planning to use it in llvm-objdump to symbolize instructions like: mov 0x206a3d(%rip),%rax # 207fe8 <__cxa_finalize@plt+0x2066a8>?

When you make a dependent llvm-objdump change, you may add the test somewhere under test/tools/llvm-objdump

seiya marked an inline comment as done.Jul 1 2019, 12:49 AM
seiya added inline comments.
llvm/include/llvm/MC/MCInstrAnalysis.h
157 ↗(On Diff #206755)

It returns bool for consistency with evaluateBranch but I think Optional is better too. What do you think about the consistency with other method definitions? I don't have a strong opinion.

seiya added a comment.Jul 1 2019, 12:55 AM

Are you planning to use it in llvm-objdump to symbolize instructions like: mov 0x206a3d(%rip),%rax # 207fe8 <__cxa_finalize@plt+0x2066a8>?

When you make a dependent llvm-objdump change, you may add the test somewhere under test/tools/llvm-objdump

Yes. The example you showed is exactly what I'm going to implement. I plan to submit a separate patch which add tests along with the implementation of the feature in llvm-objdump.

seiya updated this revision to Diff 207933.Jul 3 2019, 5:22 PM

clang-formatted

seiya updated this revision to Diff 207936.Jul 3 2019, 5:27 PM

Reverted some unrelated changes caused by clang-format.

jhenderson added inline comments.Jul 4 2019, 5:51 AM
llvm/include/llvm/MC/MCInstrAnalysis.h
157 ↗(On Diff #206755)

Personally, I have no issue in this becoming Optional to improve things (or possibly Expected, depending on what you want to do on failure). A later change could change the signature of evaluateBranch.

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
537 ↗(On Diff #207936)

Nit: trailing full stop.

seiya updated this revision to Diff 208118.Jul 4 2019, 9:53 PM
seiya marked 2 inline comments as done.
  • Use Optional
seiya marked an inline comment as done.Jul 4 2019, 9:53 PM
seiya marked an inline comment as done.
seiya added inline comments.
llvm/include/llvm/MC/MCInstrAnalysis.h
157 ↗(On Diff #206755)

That sounds good to me. I replaced bool with Optional.

I've added a couple more reviewers for you @seiya, based on the code owners file. No idea if either of them will have a chance to give this a look, but hopefully somebody will.

Sorry, but I'm not familiar with x86.

craig.topper added inline comments.Jul 19 2019, 11:22 AM
llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
531 ↗(On Diff #208118)

Should we check that getReg returns 0 for the segment register operand?

seiya updated this revision to Diff 211016.Jul 21 2019, 6:19 PM
seiya marked an inline comment as done.
  • Check that the segment register operand is not set.
seiya marked an inline comment as done.Jul 21 2019, 6:20 PM
seiya added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
531 ↗(On Diff #208118)

Yes we should. Thanks.

craig.topper added inline comments.Jul 21 2019, 10:27 PM
llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
522 ↗(On Diff #211016)

"const MCInstrDesc &MCID" or "const MCInstr &Desc" would be better.

seiya updated this revision to Diff 211405.Jul 23 2019, 8:27 PM
  • Addressed a review comment.
seiya marked an inline comment as done.Jul 23 2019, 8:27 PM
This revision is now accepted and ready to land.Jul 23 2019, 10:54 PM
MaskRay accepted this revision.Jul 24 2019, 1:38 AM
This revision was automatically updated to reflect the committed changes.