[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.

It would be nice to not pick UINT32_MAX for the unconditional condition and let each emulator picks it desired values. See inlined comments. Let me know what you think.


================
Comment at: include/lldb/Core/EmulateInstruction.h:406-410
@@ -405,4 +405,7 @@
 
-    virtual bool
-    IsInstructionConditional() { return false; }
+    // Returns a condition code in the for of uint32_t where UINT32_MAX means that the instruction
+    // is not conditional and other values specify the condition of the instruction in an
+    // architecture specififc way.
+    virtual uint32_t
+    GetInstructionCondition() { return UINT32_MAX; }
 
----------------
Add:
```
virtual uint32_t
GetUnconditionalCondition () { return 0; };
```

And change GetInstructionCondition to use the above function:

```
virtual uint32_t
GetInstructionCondition() { return GetUnconditionalCondition (); }
```

See inline comments below for more info...

================
Comment at: source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp:130
@@ +129,3 @@
+                // (UINT32_MAX means unconditional).
+                uint32_t last_condition = UINT32_MAX;
+                lldb::addr_t condition_block_start_offset = 0;
----------------
Maybe require the instruction emulator to give us an unconditional value here?

```
const uint32_t unconditional_condition = m_inst_emulator_ap->GetUnconditionalCondition();
uint32_t last_condition = unconditional_condition;
```

UINT32_MAX follows the ARM way of thinking, but other chips might have their own notion where zero is the unconditional value. It would be nice to not pick any default values for all instruction emulators.

================
Comment at: source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp:163
@@ +162,3 @@
+                        {
+                            if (m_inst_emulator_ap->GetInstructionCondition() != UINT32_MAX &&
+                                saved_unwind_states.count(current_offset) == 0)
----------------
Change to:

```
if (m_inst_emulator_ap->GetInstructionCondition() != unconditional_condition &&
```

================
Comment at: source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp:175
@@ +174,3 @@
+                            // then the then current condition then restore the condition.
+                            if (last_condition != UINT32_MAX)
+                            {
----------------
Change to:

```
if (last_condition != unconditional_condition)
```


http://reviews.llvm.org/D16814