google / binja-hexagon

GNU General Public License v2.0
104 stars 21 forks source link

LR and FP aren't eliminated in HLIL #3

Open toshipiazza opened 3 years ago

toshipiazza commented 3 years ago

The autogenerated LLIL for e.g. allocframe() and dealloc_return() manipulate LLIL_SPLIT_REG(LR, FP), and those refs don't seem to be elided in HLIL.

Example code (test_allocframe() in bn_llil_test_app):

00000020  01c09da0           { allocframe(SP,#0x8):raw } {var_8} {arg_0} {var_10}
00000024  00e00078           { R0 = #0x100 }
00000028  1ec01e96           { LR:FP = dealloc_return(FP):raw } {var_8}

LLIL:

// allocframe
   0 @ 00000020  temp29.d = SP {arg_0}
   1 @ 00000020  temp100.d = temp29.d - 8
   2 @ 00000020  [temp100.d {var_8}].q = LR:FP
   3 @ 00000020  FP = temp100.d
   4 @ 00000020  temp29.d = temp100.d - 8 {var_10}
   5 @ 00000020  SP = temp29.d
// r0 = 0x100
   6 @ 00000024  temp0.d = 0x100
   7 @ 00000024  R0 = temp0.d
// deallocframe
   8 @ 00000028  temp100.d = FP {var_8}
   9 @ 00000028  temp101.q = [temp100.d {var_8}].q
  10 @ 00000028  temp30.q = temp101.q
  11 @ 00000028  SP = temp100.d + 8
  12 @ 00000028  LR:FP = temp30.q
  13 @ 00000028  <return> jump(LR)

Resulting HLIL:

00000028      int32_t FP
00000028      int32_t FP_1
00000028      int32_t LR
00000028      int32_t LR_1
00000028      LR_1:FP_1 = LR:FP
00000028      return 0x100

I'd expect output more like

00000028      return 0x100

Looking at a different x86 binary, it seems that RBP and all callee-saved registers are eliminated somewhere between LLIL and MLIL.

toshipiazza commented 3 years ago

Another curious thing is that, when I open bn_llil_test_app for the first time in binja, test_allocframe() infers FP as the first argument (I changed that to void in the snippets above). Specifically, binja infers

int32_t test_allocframe(int32_t arg1 @ FP)

This happens even when I add FP to GetCalleeSavedRegisters in arch_hexagon.cc

toshipiazza commented 3 years ago

I'm pretty sure the issue lies with Binja's analysis. Either it expects callee-saved regs and LR/FP to be manipulated with LLIL push and pops, or it cannot handle LLIL_SPLIT_REG and LLIL_SET_SPLIT_REG.

To be sure, I modified lift_L4_return() and lift_S2_allocframe() to use straightforward push and pop's, and produced the following IL's:

LLIL:

   0 @ 00000020  push(LR)
   1 @ 00000020  push(FP)
   2 @ 00000020  FP = SP {__saved_FP}
   3 @ 00000020  SP = SP - 8
   4 @ 00000024  temp0.d = 0x100
   5 @ 00000024  R0 = temp0.d
   6 @ 00000028  SP = FP
   7 @ 00000028  FP = pop
   8 @ 00000028  LR = pop
   9 @ 00000028  <return> jump(LR)

HLIL:

00000028      return 0x100

FP is no longer inferred as an implicit argument to the function, as well.

cfircohen commented 3 years ago

Yeah, I also encountered optimization problems with LLIL_SPLIT_REG.

TempReg::CopyFromTemp in plugin/packet_context.cc also uses split registers, so I experimented a bit, and changed the split reg computation to two SetRegisters with a logical shift:

void TempReg::CopyFromTemp(BinaryNinja::LowLevelILFunction &il) {
  ExprId expr;
  if (size_ == 1) {
    expr = il.SetRegister(1, reg_, il.Register(1, LLIL_TEMP(reg_)));
    il.AddInstruction(expr);
  } else if (size_ == 4) {
    expr = il.SetRegister(4, reg_, il.Register(4, LLIL_TEMP(reg_)));
    il.AddInstruction(expr);
  } else {
    CHECK_EQ(size_, 8);

    il.AddInstruction(il.SetRegister(
        4, reg_, il.LowPart(4, il.Register(8, LLIL_TEMP(reg_)))));
    il.AddInstruction(il.SetRegister(
        4, reg_ + 1,
        il.LowPart(4, il.LogicalShiftRight(8, il.Register(8, LLIL_TEMP(reg_)),
                                           il.Const(1, 32)))));
  }
}

However, I wasn't too happy with the results, so I didn't push the change.