Skip to content

fix pop instruction to correctly handle %rsp as destination#89

Open
andres-erbsen wants to merge 2 commits intomainfrom
andreser/pop-rsp
Open

fix pop instruction to correctly handle %rsp as destination#89
andres-erbsen wants to merge 2 commits intomainfrom
andreser/pop-rsp

Conversation

@andres-erbsen
Copy link
Copy Markdown
Collaborator

No description provided.

@andres-erbsen andres-erbsen changed the title Fix pop instruction to correctly handle %rsp as destination fix pop instruction to correctly handle %rsp as destination Apr 18, 2026
Copy link
Copy Markdown
Collaborator

@protz protz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.felixcloutier.com/x86/pop says "Loads the value from the top of the stack to the location specified with the destination operand (or explicit opcode) and then increments the stack pointer."

Your patch seems to do the opposite. Am I missing something?

@andres-erbsen
Copy link
Copy Markdown
Collaborator Author

andres-erbsen commented Apr 21, 2026

Indeed. But the test fails without this change and passes with it (even with SDE from #87).

Comment on lines +4 to +5
pushq %rsp
popq (%rsp)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even more fun: what does this one do?

@andres-erbsen
Copy link
Copy Markdown
Collaborator Author

After thinking about it some more, I suspect the sentence in the documentation exists to order steps 1 and 2 below, and is just incorrect when read as ordering steps 2 and 4.

  1. load-address calculation may use rsp
  2. rsp is incremented
  3. destination-address calculation may use rsp
  4. the destination is written with the loaded value (not rsp)

It would of course be great if we could find an authoritative reference to confirm this behavior.

@protz
Copy link
Copy Markdown
Collaborator

protz commented Apr 21, 2026

As long as Intel SDE agrees I think we're good? Thanks for investigating.

But I'm still confused because the pseudocode says:

    ELSE IF StackAddrSize = 64
        THEN
                IF OperandSize = 64
                    THEN
                        DEST := SS:RSP; (* Copy quadword *)
                        RSP := RSP + 8;

let's talk about it today

@protz
Copy link
Copy Markdown
Collaborator

protz commented Apr 24, 2026

@lukaszobernig mentioned this NationalSecurityAgency/ghidra#4282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants