microsoft / llvm-mctoll

llvm-mctoll
Other
816 stars 125 forks source link

[MCDisassembler] Disambiguate Size parameter in tryAddingSymbolicOper… #172

Closed sv99 closed 2 years ago

sv99 commented 2 years ago

…and()

MCSymbolizer::tryAddingSymbolicOperand() overloaded the Size parameter to specify either the instruction size or the operand size depending on the architecture. However, for proper symbolic disassembly on X86, we need to know both sizes, as an instruction can have two operands, and the instruction size cannot be reliably calculated based on the operand offset and its size. Hence, split Size into OpSize and InstSize.

For X86, the new interface allows to fix a couple of issues:

https://github.com/llvm/llvm-project/commit/bed9efed71b954047aa11d5ed02af433dd9971cf

ghost commented 2 years ago

CLA assistant check
All CLA requirements met.

aaronsm commented 2 years ago

Thanks for the PR. Please add a test case.

MachODump.cpp comes from LLVM. I think the right thing is to fix this issue upstream then we can pull the change from llvm-project into mctoll.

https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objdump/MachODump.cpp

sv99 commented 2 years ago

This PR based on the current state of the llvm-objdump/MachODump.cpp.

bharadwajy commented 2 years ago

Thanks for the PR. I had these changes in my sandbox for a while along with some others.

bharadwajy commented 2 years ago

Reverted this change as it needs the tool to successfully build and pass tests using llvm-project commit later than that currently listed in llvm-project-git-commit-to-use.txt.

I forgot that I was working with a later git commit of llvm-project than supported currently by llvm-mctoll and that was the reason I had the same changes in my sandbox.

Will add it once tool build and test succeeds with later commit of llvm-project.