Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Correct branch analysis truncation #14027

Merged
merged 2 commits into from
Jan 30, 2021

Conversation

unknownbrackets
Copy link
Collaborator

@unknownbrackets unknownbrackets commented Jan 30, 2021

#14017 broke these functions, by truncating the signed offset to 16 bits (discarding the two most significant bits.) Only matters for larger branch jumps, but I don't want the debugger / function scanning / stack walk to get confused.

For clarity, just to explain since #14017 got merged:

    ((signed short)(op & 0xFFFF) << 2)

    Means take the IMM16 16-bit immediate value from the instruction:
    XXXXXXXX XXXXXXXX 10101010 10101010

    Then sign extend that value:
    11111111 11111111 10101010 10101010

    And shift left 2:
    11111111 11111110 10101010 10101000

In contrast:

    ((signed short)((op & 0xFFFF) << 2))

    Means take the IMM16 16-bit immediate value from the instruction:
    XXXXXXXX XXXXXXXX 10101010 10101010

    Then zero extend that value as unsigned:
    00000000 00000000 10101010 10101010

    And shift left 2:
    00000000 00000010 10101010 10101000

    And truncate it to 16-bits:
    XXXXXXXX XXXXXXXX 10101010 10101000

    And finally sign extend based on the bit that incorectly became the sign:
    11111111 11111111 10101010 10101000

Which could result in values becoming incorrectly unsigned or signed after the truncation, as well as reducing the magnitude incorrectly.

-[Unknown]

@unknownbrackets unknownbrackets added this to the v1.11.0 milestone Jan 30, 2021
@hrydgard
Copy link
Owner

hrydgard commented Jan 30, 2021

Good catch... I should have been more suspicious. @glebm

These casts are tricky. Maybe should even be replaced with explicit sign extension functions...

@hrydgard hrydgard merged commit c84ddaa into hrydgard:master Jan 30, 2021
@unknownbrackets unknownbrackets deleted the mips-cleanup branch January 30, 2021 20:05
@glebm
Copy link
Contributor

glebm commented Jan 30, 2021

A helper function for this would be great, with this explanation as a comment!

I've encountered this stuff before but always get confused by it after not seeing it for a few years.

@unknownbrackets
Copy link
Collaborator Author

Yeah, it's always ugly to have two casts next to each other. I guess we could put something in Common somewhere to do the equivalent of (uint32_t)(int32_t)(int16_t)(value & 0xFFFF) like uint32_t SignExtend16To32(uint32_t)...

-[Unknown]

@hrydgard
Copy link
Owner

Yes, I'm starting to think we should do exactly that... there's quite a few places where we would be able to use that to make code nicer.

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.

3 participants