Improve conditional opcode handling in emulation based unwinding

[tberghammer]

Summary

Improve conditional opcode handling in emulation based unwinding

Don't change the CFI information when a conditional instruction is emulated (eg.: popeq {r0, pc}) because the CFI for the next instruction should be the same as the CFI for the current instruction.

[jasonmolenda]

looks good to me.

lldb/trunk/include/lldb/Core/EmulateInstruction.h

public:
     virtual bool
     SetTargetTriple (const ArchSpec &arch) = 0;
     
     virtual bool 
     ReadInstruction () = 0;
 
     virtual bool
     EvaluateInstruction (uint32_t evaluate_options) = 0;
-    
     virtual bool
+    IsInstructionConditional() { return false; }
+
+    virtual bool
     TestEmulation (Stream *out_stream, ArchSpec &arch, OptionValueDictionary *test_data) = 0;
 
     virtual bool
     GetRegisterInfo (lldb::RegisterKind reg_kind, uint32_t reg_num, RegisterInfo &reg_info) = 0;
 
     //----------------------------------------------------------------------
     // Optional overrides
     //----------------------------------------------------------------------

lldb/trunk/source/Plugins/Instruction/ARM/EmulateInstructionARM.h

//    }
     virtual bool 
     ReadInstruction ();
     
     virtual bool
     SetInstruction (const Opcode &insn_opcode, const Address &inst_addr, Target *target);
 
     virtual bool
     EvaluateInstruction (uint32_t evaluate_options);
-    
+    bool
+    IsInstructionConditional() override;
+
     virtual bool
     TestEmulation (Stream *out_stream, ArchSpec &arch, OptionValueDictionary *test_data);
 
     virtual bool
     GetRegisterInfo (lldb::RegisterKind reg_kind, uint32_t reg_num, RegisterInfo &reg_info);
     
 
     virtual bool
     CreateFunctionEntryUnwind (UnwindPlan &unwind_plan);
 
     uint32_t
     ArchVersion();
 
     bool
-    ConditionPassed (const uint32_t opcode, 
-                     bool *is_conditional = NULL);  // Filled in with true if the opcode is a conditional opcode
-                                                    // Filled in with false if the opcode is always executed
+    ConditionPassed (const uint32_t opcode);
 
     uint32_t
     CurrentCond (const uint32_t opcode);
 
     // InITBlock - Returns true if we're in Thumb mode and inside an IT Block.
     bool InITBlock();
 
     // LastInITBlock - Returns true if we're in Thumb mode and the last instruction inside an IT Block.

lldb/trunk/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp

if (ConditionPassed())
 
         if (registers<15> == '1') // Only possible for encoding A1 or A2 
             MemA[address,4] = PCStoreValue();
         
         SP = SP - 4*BitCount(registers);
     }
 #endif
 
-    bool conditional = false;
     bool success = false;
-    if (ConditionPassed(opcode, &conditional))
+    if (ConditionPassed(opcode))
     {
         const uint32_t addr_byte_size = GetAddressByteSize();
         const addr_t sp = ReadCoreReg (SP_REG, &success);
         if (!success)
             return false;
         uint32_t registers = 0;
         uint32_t Rt; // the source register
         switch (encoding) {
if (ConditionPassed(opcode))
         default:
             return false;
         }
         addr_t sp_offset = addr_byte_size * BitCount (registers);
         addr_t addr = sp - sp_offset;
         uint32_t i;
         
         EmulateInstruction::Context context;
-        if (conditional)
-            context.type = EmulateInstruction::eContextRegisterStore;
-        else
-            context.type = EmulateInstruction::eContextPushRegisterOnStack;
         RegisterInfo reg_info;
         RegisterInfo sp_reg;
         GetRegisterInfo (eRegisterKindDWARF, dwarf_sp, sp_reg);
         for (i=0; i<15; ++i)
         {
             if (BitIsSet (registers, i))
             {
                 GetRegisterInfo (eRegisterKindDWARF, dwarf_r0 + i, reg_info);
if (ConditionPassed())
                 LoadWritePC(MemA[address,4]);
         if registers<13> == '0' then SP = SP + 4*BitCount(registers);
         if registers<13> == '1' then SP = bits(32) UNKNOWN;
     }
 #endif
 
     bool success = false;
 
-    bool conditional = false;
-    if (ConditionPassed(opcode, &conditional))
+    if (ConditionPassed(opcode))
     {
         const uint32_t addr_byte_size = GetAddressByteSize();
         const addr_t sp = ReadCoreReg (SP_REG, &success);
         if (!success)
             return false;
         uint32_t registers = 0;
         uint32_t Rt; // the destination register
         switch (encoding) {
if (ConditionPassed(opcode))
         default:
             return false;
         }
         addr_t sp_offset = addr_byte_size * BitCount (registers);
         addr_t addr = sp;
         uint32_t i, data;
         
         EmulateInstruction::Context context;
-        if (conditional)
-            context.type = EmulateInstruction::eContextRegisterLoad;
-        else
-            context.type = EmulateInstruction::eContextPopRegisterOffStack;
         
         RegisterInfo sp_reg;
         GetRegisterInfo (eRegisterKindDWARF, dwarf_sp, sp_reg);
 
         for (i=0; i<15; ++i)
         {
             if (BitIsSet (registers, i))
             {
if (ConditionPassed())
         EncodingSpecificOperations();
         offset_addr = if add then (R[n] + imm32) else (R[n] - imm32);
         address = if index then offset_addr else R[n];
         MemU[address,4] = if t == 15 then PCStoreValue() else R[t];
         if wback then R[n] = offset_addr;
     }
 #endif
 
-    bool conditional = false;
     bool success = false;
-    if (ConditionPassed(opcode, &conditional))
+    if (ConditionPassed(opcode))
     {
         const uint32_t addr_byte_size = GetAddressByteSize();
         const addr_t sp = ReadCoreReg (SP_REG, &success);
         if (!success)
             return false;
         uint32_t Rt; // the source register
         uint32_t imm12;
         uint32_t Rn;  // This function assumes Rn is the SP, but we should verify that.
if (ConditionPassed(opcode))
                   
         addr_t addr;
         if (index)
             addr = offset_addr;
         else
             addr = sp;
         
         EmulateInstruction::Context context;
-        if (conditional)
-            context.type = EmulateInstruction::eContextRegisterStore;
-        else
-            context.type = EmulateInstruction::eContextPushRegisterOnStack;
         RegisterInfo sp_reg;
         RegisterInfo dwarf_reg;
 
         GetRegisterInfo (eRegisterKindDWARF, dwarf_sp, sp_reg);
         GetRegisterInfo (eRegisterKindDWARF, dwarf_r0 + Rt, dwarf_reg);
         context.SetRegisterToRegisterPlusOffset ( dwarf_reg, sp_reg, addr - sp);
         if (Rt != 15)
         {
if (ConditionPassed())
                 // Store as two word-aligned words in the correct order for current endianness.
                 MemA[address,4] = if BigEndian() then D[d+r]<63:32> else D[d+r]<31:0>;
                 MemA[address+4,4] = if BigEndian() then D[d+r]<31:0> else D[d+r]<63:32>;
                 address = address+8;
     }
 #endif
 
     bool success = false;
-    bool conditional = false;
-    if (ConditionPassed(opcode, &conditional))
+    if (ConditionPassed(opcode))
     {
         const uint32_t addr_byte_size = GetAddressByteSize();
         const addr_t sp = ReadCoreReg (SP_REG, &success);
         if (!success)
             return false;
         bool single_regs;
         uint32_t d;     // UInt(D:Vd) or UInt(Vd:D) starting register
         uint32_t imm32; // stack offset
if (ConditionPassed(opcode))
         }
         uint32_t start_reg = single_regs ? dwarf_s0 : dwarf_d0;
         uint32_t reg_byte_size = single_regs ? addr_byte_size : addr_byte_size * 2;
         addr_t sp_offset = imm32;
         addr_t addr = sp - sp_offset;
         uint32_t i;
         
         EmulateInstruction::Context context;
-        if (conditional)
-            context.type = EmulateInstruction::eContextRegisterStore;
-        else
-            context.type = EmulateInstruction::eContextPushRegisterOnStack;
+
         RegisterInfo dwarf_reg;
         RegisterInfo sp_reg;
         GetRegisterInfo (eRegisterKindDWARF, dwarf_sp, sp_reg);
         for (i=0; i<regs; ++i)
         {
             GetRegisterInfo (eRegisterKindDWARF, start_reg + d + i, dwarf_reg);
             context.SetRegisterToRegisterPlusOffset ( dwarf_reg, sp_reg, addr - sp);
             // uint64_t to accommodate 64-bit registers.
if (ConditionPassed())
             for r = 0 to regs-1
                 word1 = MemA[address,4]; word2 = MemA[address+4,4]; address = address+8;
                 // Combine the word-aligned words in the correct order for current endianness.
                 D[d+r] = if BigEndian() then word1:word2 else word2:word1;
     }
 #endif
 
     bool success = false;
-    bool conditional = false;
-    if (ConditionPassed(opcode, &conditional))
+    if (ConditionPassed(opcode))
     {
         const uint32_t addr_byte_size = GetAddressByteSize();
         const addr_t sp = ReadCoreReg (SP_REG, &success);
         if (!success)
             return false;
         bool single_regs;
         uint32_t d;     // UInt(D:Vd) or UInt(Vd:D) starting register
         uint32_t imm32; // stack offset
if (ConditionPassed(opcode))
         uint32_t start_reg = single_regs ? dwarf_s0 : dwarf_d0;
         uint32_t reg_byte_size = single_regs ? addr_byte_size : addr_byte_size * 2;
         addr_t sp_offset = imm32;
         addr_t addr = sp;
         uint32_t i;
         uint64_t data; // uint64_t to accommodate 64-bit registers.
         
         EmulateInstruction::Context context;
-        if (conditional)
-            context.type = EmulateInstruction::eContextRegisterLoad;
-        else
-            context.type = EmulateInstruction::eContextPopRegisterOffStack;
+
         RegisterInfo dwarf_reg;
         RegisterInfo sp_reg;
         GetRegisterInfo (eRegisterKindDWARF, dwarf_sp, sp_reg);
         for (i=0; i<regs; ++i)
         {
             GetRegisterInfo (eRegisterKindDWARF, start_reg + d + i, dwarf_reg);
             context.SetRegisterPlusOffset (sp_reg, addr - sp);
             data = MemARead(context, addr, reg_byte_size, 0, &success);
if ConditionPassed()
             LoadWritePC (MemA[address, 4]);
                   
         if wback && registers<n> == '0' then R[n] = R[n] + 4 * BitCount (registers);
         if wback && registers<n> == '1' then R[n] = bits(32) UNKNOWN; // Only possible for encoding A1
 
 #endif
             
     bool success = false;
-    bool conditional = false;
-    if (ConditionPassed(opcode, &conditional))
+    if (ConditionPassed(opcode))
     {
         uint32_t n;
         uint32_t registers = 0;
         bool wback;
         const uint32_t addr_byte_size = GetAddressByteSize();
         switch (encoding)
         {
             case eEncodingT1:
if (ConditionPassed(opcode))
                   
         for (int i = 0; i < 14; ++i)
         {
             if (BitIsSet (registers, i))
             {
                 context.type = EmulateInstruction::eContextRegisterPlusOffset;
                 context.SetRegisterPlusOffset (dwarf_reg, offset);
                 if (wback && (n == 13)) // Pop Instruction
-                {
-                    if (conditional)
-                        context.type = EmulateInstruction::eContextRegisterLoad;
-                    else
-                        context.type = EmulateInstruction::eContextPopRegisterOffStack;
-                }
 
                 // R[i] = MemA [address, 4]; address = address + 4;
                 uint32_t data = MemARead (context, base_address + offset, addr_byte_size, 0, &success);
                 if (!success)
                     return false;
                   
                 if (!WriteRegisterUnsigned (context, eRegisterKindDWARF, dwarf_r0 + i, data))
                     return false;

 
 uint32_t
 EmulateInstructionARM::ArchVersion ()
 {
     return m_arm_isa;
 }
 
 bool
-EmulateInstructionARM::ConditionPassed (const uint32_t opcode, bool *is_conditional)
+EmulateInstructionARM::ConditionPassed (const uint32_t opcode)
 {
    // If we are ignoring conditions, then always return true.
    // this allows us to iterate over disassembly code and still
    // emulate an instruction even if we don't have all the right
    // bits set in the CPSR register...
     if (m_ignore_conditions)
         return true;
-    
-    if (is_conditional)
-        *is_conditional = true;
-
     const uint32_t cond = CurrentCond (opcode);
-    
     if (cond == UINT32_MAX)
         return false;
     
     bool result = false;
     switch (UnsignedBits(cond, 3, 1))
     {
     case 0: 
     if (m_opcode_cpsr == 0)
case 6:
             bool n = (m_opcode_cpsr & MASK_CPSR_N);
             bool v = (m_opcode_cpsr & MASK_CPSR_V);
             result = n == v && ((m_opcode_cpsr & MASK_CPSR_Z) == 0);
         }
         break;
     case 7: 
         // Always execute (cond == 0b1110, or the special 0b1111 which gives
         // opcodes different meanings, but always means execution happens.
-        if (is_conditional)
-            *is_conditional = false;
         return true;
     }
 
     if (cond & 1)
         result = !result;
     return result;
 }
 
if (auto_advance_pc)
                 return false;
                 
         }
     }
     return true;
 }
 
 bool
+EmulateInstructionARM::IsInstructionConditional()
+{
+    const uint32_t cond = CurrentCond (m_opcode.GetOpcode32());
+    return cond != 0xe && cond != 0xf && cond != UINT32_MAX;
+}
+
+bool
 EmulateInstructionARM::TestEmulation (Stream *out_stream, ArchSpec &arch, OptionValueDictionary *test_data)
 {
     if (!test_data)
     {
         out_stream->Printf ("TestEmulation: Missing test data.\n");
         return false;
     }
     

lldb/trunk/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.h

protected:
 
     uint32_t
     GetFramePointerRegisterNumber() const;
 
     bool
     BranchTo (const Context &context, uint32_t N, lldb::addr_t target);
 
     bool
-    ConditionHolds (const uint32_t cond, bool *is_conditional = nullptr);
+    ConditionHolds (const uint32_t cond);
 
     bool
     UsingAArch32 ();
 
     bool
     Emulate_addsub_imm (const uint32_t opcode);
 
     bool

lldb/trunk/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp

#endif
 
     if (!WriteRegisterUnsigned (context, eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC, addr))
         return false;
 
     return true;
 }
 
 bool
-EmulateInstructionARM64::ConditionHolds (const uint32_t cond, bool *is_conditional)
+EmulateInstructionARM64::ConditionHolds (const uint32_t cond)
 {
    // If we are ignoring conditions, then always return true.
    // this allows us to iterate over disassembly code and still
    // emulate an instruction even if we don't have all the right
    // bits set in the CPSR register...
     if (m_ignore_conditions)
         return true;
-    
-    if (is_conditional)
-        *is_conditional = true;
-  
     bool result = false;
     switch (UnsignedBits(cond, 3, 1))
     {
     case 0:
         result = (m_opcode_pstate.Z == 1);
         break;
     case 1:
         result = (m_opcode_pstate.C == 1);
case 4:
         break;
     case 5:
         result = (m_opcode_pstate.N == m_opcode_pstate.V);
         break;
     case 6:
         result = (m_opcode_pstate.N == m_opcode_pstate.V && m_opcode_pstate.Z == 0);
         break;
     case 7:
-        result = true;
-        if (is_conditional)
-            *is_conditional = false;
-        break;
+        // Always execute (cond == 0b1110, or the special 0b1111 which gives
+        // opcodes different meanings, but always means execution happens.
+        return true;
     }
 
-    if (cond & 1 && cond != 15)
+    if (cond & 1)
         result = !result;
     return result;
 }
 
 bool
 EmulateInstructionARM64::Emulate_addsub_imm (const uint32_t opcode)
 {
     // integer d = UInt(Rd);

lldb/trunk/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp

if (log && log->GetVerbose ())
         StreamString strm;
         strm.Printf ("UnwindAssemblyInstEmulation::WriteRegister (name = \"%s\", value = ", reg_info->name);
         reg_value.Dump(&strm, reg_info, false, false, eFormatDefault);
         strm.PutCString (", context = ");
         context.Dump(strm, instruction);
         log->PutCString(strm.GetData());
     }
 
+    if (!instruction->IsInstructionConditional())
         SetRegisterValue (*reg_info, reg_value);
 
     switch (context.type)
     {
         case EmulateInstruction::eContextInvalid:
         case EmulateInstruction::eContextReadOpcode:
         case EmulateInstruction::eContextImmediate:
         case EmulateInstruction::eContextAdjustBaseRegister:
         case EmulateInstruction::eContextRegisterPlusOffset:
//            }
                 {
                     m_forward_branch_offset = context.info.signed_immediate;
                 }
             }
             break;
 
         case EmulateInstruction::eContextPopRegisterOffStack:
             {
+                if (!instruction->IsInstructionConditional())
+                {
                     const uint32_t reg_num = reg_info->kinds[m_unwind_plan_ptr->GetRegisterKind()];
                     const uint32_t generic_regnum = reg_info->kinds[eRegisterKindGeneric];
                     if (reg_num != LLDB_INVALID_REGNUM && generic_regnum != LLDB_REGNUM_GENERIC_SP)
                     {
                         m_curr_row->SetRegisterLocationToSame (reg_num, /*must_replace*/ false);
                         m_curr_row_modified = true;
                     }
                 }
+            }
             break;
 
         case EmulateInstruction::eContextSetFramePointer:
-            if (!m_fp_is_cfa)
+            if (!m_fp_is_cfa && !instruction->IsInstructionConditional())
             {
                 m_fp_is_cfa = true;
                 m_cfa_reg_info = *reg_info;
                 const uint32_t cfa_reg_num = reg_info->kinds[m_unwind_plan_ptr->GetRegisterKind()];
                 assert (cfa_reg_num != LLDB_INVALID_REGNUM);
                 m_curr_row->GetCFAValue().SetIsRegisterPlusOffset(cfa_reg_num, m_initial_sp -
                         reg_value.GetAsUInt64());
                 m_curr_row_modified = true;
             }
             break;
 
         case EmulateInstruction::eContextAdjustStackPointer:
             // If we have created a frame using the frame pointer, don't follow
             // subsequent adjustments to the stack pointer.
-            if (!m_fp_is_cfa)
+            if (!m_fp_is_cfa && !instruction->IsInstructionConditional())
             {
                 m_curr_row->GetCFAValue().SetIsRegisterPlusOffset(
                         m_curr_row->GetCFAValue().GetRegisterNumber(),
                         m_initial_sp - reg_value.GetAsUInt64());
                 m_curr_row_modified = true;
             }
             break;
     }
     return true;
 }
 
 

lldb/trunk/test/functionalities/unwind/standard/TestStandardUnwind.py

def standard_unwind_tests (self):
         #                         functions we will immediately do a step-out when we hit them.
 
         triple = self.dbg.GetSelectedPlatform().GetTriple()
         if re.match("arm-.*-.*-android", triple):
             base_function_names = [
                 "_start",                # Base function on the stack
                 "__memcpy_base",         # Function reached by a fall through from the previous function
                 "__memcpy_base_aligned", # Function reached by a fall through from the previous function
-                "__subdf3",              # __aeabi_ui2d jumps into the middle of the function. Possibly missing symbol?
             ]
             no_step_function_names = [
                 "__sync_fetch_and_add_4", # Calls into a special SO where we can't set a breakpoint
                 "pthread_mutex_lock",     # Uses ldrex and strex what interferes with the software single stepping
                 "pthread_mutex_unlock",   # Uses ldrex and strex what interferes with the software single stepping
                 "pthread_once",           # Uses ldrex and strex what interferes with the software single stepping
             ]
         elif re.match("aarch64-.*-.*-android", triple):