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

Code with FDIVR decompiled wrong #293

Closed
DennisD2 opened this issue May 5, 2018 · 2 comments
Closed

Code with FDIVR decompiled wrong #293

DennisD2 opened this issue May 5, 2018 · 2 comments

Comments

@DennisD2
Copy link

DennisD2 commented May 5, 2018

Hello,
it looks that code containing FDIVR is decompiled the same as FDIV, which lead to wrong results.
IDA shows me the following code:

mov     edx, [ebp+a1]
fild    [edx+S.deflt_pt_cnt]
fsub    ds:dbl_100160F0
fdivr   [ebp+v6]
fstp    [ebp+var_18]

Which is decompiled to something like this:

int32_t v30 = *v29; // 0x100018aa
*v9 = v30;
*v26 = (float64_t)((float80_t)(v30 - 1) / v6); // pattern '(a-1)/b'

Correct would be something like this:
*v26 = v6 / (v30- 1.0); // pattern 'b/(a-1)'

Itt looks like the opcode FDIVR is decompiled like opcode FDIV.

Decompiled Function is here https://github.com/DennisD2/tomorrow/blob/master/src/morrow/orig/mtcsa32.dll.c, function_10001718(). Original DLL here: https://github.com/DennisD2/tomorrow/blob/master/src/morrow/orig/lib/Mtcsa32.dll .

DennisD2 pushed a commit to DennisD2/tomorrow that referenced this issue May 5, 2018
for maybe later use. It was found that retdec decompiles the code
containing FDIVR opcode the wrong way around...
See avast/retdec#293.
@PeterMatula PeterMatula self-assigned this May 14, 2018
@PeterMatula
Copy link
Collaborator

PeterMatula commented May 14, 2018

I can confirm this is a bug. Operands are in an incorrect order at least in the following case:

DC /7 | FDIVR m64fp | Divide m64fp by ST(0) and store result in ST(0)

I don't want to hotfix just this problem, because this might be an issue all across x86 FPU instructions. The thing is that in LLVM IR generation, operands are reversed as one would expect, but the output is still wrong:

// X86_INS_FDIV
auto* fdiv = irb.CreateFDiv(op0, op1);
// X86_INS_FDIVR
auto* fdiv = irb.CreateFDiv(op1, op0);

It may have something to do with an order of operands as get by loadOpFloatingUnaryTop() call. I will have to check correct generation of all instructions using this method.

Thanks for the report, this is potentially a major issue, I will do my best to properly fix it as soon as possible.

@PeterMatula
Copy link
Collaborator

The current output looks different than the lines you posted, but I think these are the relevant lines:

Before fix:

*(float64_t *)(a1 + 24) = (float64_t)((float80_t)(v33 - 1) / v3);

After fix:

*(float64_t *)(a1 + 24) = (float64_t)(v3 / (float80_t)(v34 - 1));

As expected, there were problems with many instructions other than FDIVR. I fixed and added meticulous unit tests for all the currently supported x86 FPU instructions. So from the viewpoint of capstone2llvmir library, which generates the LLVM IR code, this is done. RetDec's FPU support is still quite naive and incomplete, but I will not be solving that in this issue. Hopefully, one student will make it better as part of his bachelor thesis - see milestone 1 for more details.

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

No branches or pull requests

2 participants