Fix compilation with MSVC: arm64/disassembler/decode_scratchpad.c #6574
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MSVC shows several warnings, but this one is shown as an error, preventing compilation.
|
Unfortunately, I had to revert this commit as it broke the arm64test.py output for LIFT MISMATCH AT TEST 3157!
input: c31d1c33 bfi w3, w14, #0xffffffe4, #0x8
test: 331C1DC3 bfi w3, w14, #0xffffffe4, #0x8
expected: LLIL_SET_REG.d(w3,LLIL_OR.d(LLIL_AND.d(LLIL_CONST.d(0xFFFFF00F),LLIL_REG.d(w3)),LLIL_LSL.d(LLIL_AND.d(LLIL_CONST.d(0xFF),LLIL_REG.d(w14)),LLIL_CONST.b(0x4))))
actual: LLIL_SET_REG.d(w3,LLIL_OR.d(LLIL_AND.d(LLIL_CONST.d(-0xFEF00000001),LLIL_REG.d(w3)),LLIL_LSL.d(LLIL_AND.d(LLIL_CONST.d(0xFF),LLIL_REG.d(w14)),LLIL_CONST.b(0xFFFFFFE4))))
tree:
LLIL_SET_REG.d
w3
LLIL_OR.d
LLIL_AND.d
LLIL_CONST.d
-0xFEF00000001
LLIL_REG.d
w3
LLIL_LSL.d
LLIL_AND.d
LLIL_CONST.d
0xFF
LLIL_REG.d
w14
LLIL_CONST.b
0xFFFFFFE4After some closer investigation, it also turns out that this change causes the disassembly output to be incorrect, and the LLIL to be lifted incorrectly: Before/After this patch: Disassembly: c31d1c33 bfi w3, w14, #0x4, #8
c31d1c33 bfi w3, w14, #0xffffffe4, #0x8LLIL: w3 = (-0xff1 & w3) | (0xff & w14) << 4
w3 = (-0xff000000001 & w3) | (0xff & w14) << 0xffffffe4MLIL: x3 = zx.q((0xfffff00f & arg4) | zx.d(arg5) << 4)
x3 = zx.q((0xffffffff & arg4) | zx.d(arg5) << 0xe4)I have also verified that the original code does not cause a warning on the version of MSVC we use in our build infrastructure. Also, from the link you posted above, it sounds like the problem might be a bug in the version of MSVC you're using. Try adding |
|
I checked all flags and it's caused by /sdl, latest MSVC version:
https://c.godbolt.org/z/PsKrc6r5v
I also see the bug now, with the change it does a signed modulo. This is a
fix:
unsigned lsb = ((uint64_t)-(int64_t)IMMR) % 32;
Ugly, but without it it doesn't compile unless I remove that flag.
Can you run the tests with this change? Should I create a new PR, or can
you just update it?
thx
…On Thu, May 29, 2025 at 12:23 AM galenbwill ***@***.***> wrote:
*galenbwill* left a comment (Vector35/binaryninja-api#6574)
<#6574 (comment)>
Unfortunately, I had to revert this commit as it broke the arm64test.py
<https://github.com/Vector35/binaryninja-api/blob/dev/arch/arm64/arm64test.py>
output for BFI. For example:
LIFT MISMATCH AT TEST 3157!
input: c31d1c33 bfi w3, w14, #0xffffffe4, #0x8
test: 331C1DC3 bfi w3, w14, #0xffffffe4, #0x8
expected: LLIL_SET_REG.d(w3,LLIL_OR.d(LLIL_AND.d(LLIL_CONST.d(0xFFFFF00F),LLIL_REG.d(w3)),LLIL_LSL.d(LLIL_AND.d(LLIL_CONST.d(0xFF),LLIL_REG.d(w14)),LLIL_CONST.b(0x4))))
actual: LLIL_SET_REG.d(w3,LLIL_OR.d(LLIL_AND.d(LLIL_CONST.d(-0xFEF00000001),LLIL_REG.d(w3)),LLIL_LSL.d(LLIL_AND.d(LLIL_CONST.d(0xFF),LLIL_REG.d(w14)),LLIL_CONST.b(0xFFFFFFE4))))
tree:LLIL_SET_REG.d
w3
LLIL_OR.d
LLIL_AND.d
LLIL_CONST.d
-0xFEF00000001
LLIL_REG.d
w3
LLIL_LSL.d
LLIL_AND.d
LLIL_CONST.d
0xFF
LLIL_REG.d
w14
LLIL_CONST.b
0xFFFFFFE4
After some closer investigation, it also turns out that this change causes
the disassembly output to be incorrect, and the LLIL to be lifted
incorrectly:
Before/After this patch:
Disassembly:
c31d1c33 bfi w3, w14, #0x4, #8c31d1c33 bfi w3, w14, #0xffffffe4, #0x8
LLIL:
w3 = (-0xff1 & w3) | (0xff & w14) << 4w3 = (-0xff000000001 & w3) | (0xff & w14) << 0xffffffe4
MLIL:
x3 = zx.q((0xfffff00f & arg4) | zx.d(arg5) << 4)x3 = zx.q((0xffffffff & arg4) | zx.d(arg5) << 0xe4)
I have also verified that the original code does not cause a warning on
the version of MSVC we use in our build infrastructure. Also, from the link
you posted above, it sounds like the problem might be a bug in the version
of MSVC you're using.
Try adding #pragma warning(disable:4146) right above the offending line
and see if that prevents the error.
—
Reply to this email directly, view it on GitHub
<#6574 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABMDRPFMZIRUGLFGGRPXJDT3AYSMNAVCNFSM6AAAAAB2QZ7HMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSMJXGY2DOMBQGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I actually did try exactly that code when I was investigating, and yes, it does pass the tests and seems to result in semantically correct decompilation. If you update the PR, I will merge it the next time I'm pushing another change (which will probably be in the next day or so). |
|
Actually the pragma works too and it's less ugly. Created #6896 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MSVC shows several warnings, but this one is shown as an error, preventing compilation.
error C4146: unary minus operator applied to unsigned type, result still unsigned