[Lldb-commits] [PATCH] D16814: Fix handling of the arm IT instruction in the unwinder

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Yes, it was probably too complex. My main objection was the use of the UINT32_MAX as a magic number. Your UnconditionalCondition solution clears this up.


================
Comment at: include/lldb/Core/EmulateInstruction.h:390
@@ +389,3 @@
+    typedef uint32_t InstructionCondition;
+    static const InstructionCondition UnconditionalCondition = UINT32_MAX;
+
----------------
It is nice to tell when something is a constant (prefixed with "k"), a member variable (prefixed with "m_"), typenames are camel case, variables should be lower cased with underscores. That is our current coding convention.

I would like to see this be

```
    static const InstructionCondition k_unconditional_condition = UINT32_MAX;
```


================
Comment at: include/lldb/Core/EmulateInstruction.h:412
@@ -408,1 +411,3 @@
+    virtual InstructionCondition
+    GetInstructionCondition() { return UnconditionalCondition; }
 
----------------
k_unconditional_condition

================
Comment at: source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp:13653
@@ -13656,2 +13652,3 @@
     const uint32_t cond = CurrentCond (m_opcode.GetOpcode32());
-    return cond != 0xe && cond != 0xf && cond != UINT32_MAX;
+    if (cond == 0xe || cond == 0xf || cond == UINT32_MAX)
+        return EmulateInstruction::UnconditionalCondition;
----------------
Do you want to use UnconditionalCondition (or k_unconditional_condition) instead of UINT32_MAX here?

================
Comment at: source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp:13654
@@ +13653,3 @@
+    if (cond == 0xe || cond == 0xf || cond == UINT32_MAX)
+        return EmulateInstruction::UnconditionalCondition;
+    return cond;
----------------
k_unconditional_condition

================
Comment at: source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp:129
@@ +128,3 @@
+                // The architecture dependent condition code of the last processed instruction.
+                EmulateInstruction::InstructionCondition last_condition = EmulateInstruction::UnconditionalCondition;
+                lldb::addr_t condition_block_start_offset = 0;
----------------
k_unconditional_condition


http://reviews.llvm.org/D16814