pop (esp) has the wrong base for esp

[derekbruening]

From [email protected] on April 03, 2014 09:39:18

xref issue #164 xref https://code.google.com/p/drmemory/issues/detail?id=1502 Intel manual:
If the ESP register is used as a base register for addressing a destination
operand in memory, the POP instruction computes the effective address of
the operand after it increments the ESP register. For the case of a 16-bit
stack where ESP wraps to 0H as a result of the POP instruction, the
resulting location of the memory write is processor- family-specific.

But to solve this in DR we have to add a stack slot size to any existing
disp: it could be "pop 0x14(esp)" or sthg. We'd then have to decrement
the offset on encoding, and on disassembly to avoid confusion? For Intel
or AT&T modes, at least, as we do display the adjusted value for push:

0x50f34a35 51 push %ecx %esp -> %esp 0xfffffffc(%esp)[4byte]

Original issue: http://code.google.com/p/dynamorio/issues/detail?id=1410

[derekbruening]

From [email protected] on April 03, 2014 08:19:15

It can get complicated: what if client constructs level 4 instr "pop 0x18(esp)"?
All the rest of DRi#164 was much easier b/c it was all about implicit operands.
This is the first time dealing w/ this kind of thing on an explicit opnd.

then how about when compute address, pass the opcode in too
so we can special handle the pop 0x14(esp)

that's not perfectly clean wrt DRi#164 where when we disasm we show the pre-instr esp value, but it may be the lesser of two evils: hard to have it both ways
e.g., we show:
0x50f34a35 51 push %ecx %esp -> %esp 0xfffffffc(%esp)[4byte]
but for pop 0x14(esp) we won't show the true pre-instr -- but it does seem much less worse than adjusting it and trying to figure out programmer intent (adjust on encode only if still points at raw bits, and then programmer can construct 0x14 and have it stay that way? that gets really fragile)

So the consensus is to not adjust the representation and instead add some
adjustment on addr computation. Addr computation w/o opcode already doesn't
work for VSIB, so ok to add another case that needs opcode.