tberghammer added a comment.
I don't know too much about mips so I haven't checked if the emulation is
actually correct but the general concept looks good to me. I added a few
comments inline but they are mostly suggestions what you might want to consider.
================
Comment at:
source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:553-562
@@ -476,2 +552,12 @@
{ "BC1ANY4T", &EmulateInstructionMIPS64::Emulate_BC1ANY4T,
"BC1ANY4T cc, offset" },
+ { "BNZ_B", &EmulateInstructionMIPS64::Emulate_BNZB, "BNZ.b
wt,s16" },
+ { "BNZ_H", &EmulateInstructionMIPS64::Emulate_BNZH, "BNZ.h
wt,s16" },
+ { "BNZ_W", &EmulateInstructionMIPS64::Emulate_BNZW, "BNZ.w
wt,s16" },
+ { "BNZ_D", &EmulateInstructionMIPS64::Emulate_BNZD, "BNZ.d
wt,s16" },
+ { "BZ_B", &EmulateInstructionMIPS64::Emulate_BZB, "BZ.b
wt,s16" },
+ { "BZ_H", &EmulateInstructionMIPS64::Emulate_BZH, "BZ.h
wt,s16" },
+ { "BZ_W", &EmulateInstructionMIPS64::Emulate_BZW, "BZ.w
wt,s16" },
+ { "BZ_D", &EmulateInstructionMIPS64::Emulate_BZD, "BZ.d
wt,s16" },
+ { "BNZ_V", &EmulateInstructionMIPS64::Emulate_BNZV, "BNZ.V
wt,s16" },
+ { "BZ_V", &EmulateInstructionMIPS64::Emulate_BZV, "BZ.V
wt,s16" },
};
----------------
(nit): Indentation
================
Comment at:
source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3138-3140
@@ +3137,5 @@
+ bool success = false, branch_hit = true;
+ uint32_t wt;
+ int64_t offset, pc, target;
+ RegisterValue reg_value;
+ uint8_t * ptr = NULL;
----------------
Please initialize these variables
================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3160
@@ +3159,3 @@
+ case 1:
+ if((*ptr == 0 && bnz) || (*ptr != 0 && !bnz) )
+ branch_hit = false;
----------------
You can possibly write it in the following way, but I am not sure which one is
the cleaner:
```
if((*ptr == 0) == bnz)
break;
```
================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3187
@@ +3186,3 @@
+ Context context;
+ context.type = eContextInvalid;
+
----------------
Using eContextInvalid as context type is fine for single stepping but if you
want these instructions to be handled correctly during emulation based stack
unwinding then you have to use eContextAbsoluteBranchRegister or
eContextRelativeBranchImmediate with the correct data (it is used by the branch
following code in the unwinder)
================
Comment at:
source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3211-3213
@@ +3210,5 @@
+ bool success = false;
+ uint32_t wt;
+ int64_t offset, pc, target;
+ llvm::APInt wr_val;
+ llvm::APInt fail_value = llvm::APInt::getMaxValue(128);
----------------
Please initialize these variables
================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3230
@@ +3229,3 @@
+
+ if((llvm::APInt::isSameValue(zero_value, wr_val) && !bnz) ||
(!llvm::APInt::isSameValue(zero_value, wr_val) && bnz))
+ target = pc + offset;
----------------
You can possibly write it in the following way, but I am not sure which one is
the cleaner:
```
if(llvm::APInt::isSameValue(zero_value, wr_val) != bnz)
...
```
================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3236
@@ +3235,3 @@
+ Context context;
+ context.type = eContextInvalid;
+
----------------
Using eContextInvalid as context type is fine for single stepping but if you
want these instructions to be handled correctly during emulation based stack
unwinding then you have to use eContextAbsoluteBranchRegister or
eContextRelativeBranchImmediate with the correct data (it is used by the branch
following code in the unwinder)
Repository:
rL LLVM
http://reviews.llvm.org/D12356
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
