[MIPS] Avoid breakpoint in delay slot

[bhushan]

Summary

In MIPS, when a breakpoint is hit in a delay slot then the PC points to the previous branch/jump instruction. In this case, CAUSE.BD bit is set and we can correct the PC accordingly.
However doing a single step at this point will continue execution from the current PC and not from the target of previous branch/jump instruction.
Solution to this is to not allow breakpoint in a delay slot and move it to previous branch/jump instruction (which will have same effect).

When user tries to set breakpoint by address then this patch checks if the instruction at that address is a delay slot instruction and if it is then the breakpoint is moved to its previous instruction.

[jingham]

I wish we had a better abstraction for machine-specific behaviors than ArchSpec, since it seems sad to put this machine specific a bit of business in Target.cpp, but I don't think you should have to tackle that to get this fix in.

However, I think you came in at too low a level by making the interface about delay slots. Rather, there should be a function Target::GetBreakableLoadAddress (analogous to GetOpcodeLoadAddress & GetCallableLoadAddress) that encapsulated calculating the new breakpoint address and CreateBreakpoint should call that. After all, maybe there will be other reasons why "you want to set a breakpoint on address X but we know Y is a better address" might happen and it would be better to hide the specific reason away as much as possible.

Also, since this will get called for every breakpoint address we set, please don't do any work in this function till you've decided you are on a machine that would require it.

Also, for you own sanity, I would suggest putting some logging (under the breakpoint channel) summarizing what you did to the address. Someday, you'll get some weird bug report from somebody whose breakpoints are going wrong, and then you'll wish too late that you knew what you thought you were doing...

[bhushan]

Addressed review comments.

1. Introduced Target::GetBreakableLoadAddress() instead of Target::AdjustBreakpointInDelaySlot() so that it should look more generic and targets can calculate the new breakpoint address based on whatever reason applicable to them (not restricting it to a delay-slot only).

2. Target::GetBreakableLoadAddress() does not do any work unless a target requires it.

3. Logging is enabled (under the breakpoint channel) and now mentions a reason why we changed the original breakpoint address. (In case of MIPS, the reason is a delay slot).

[clayborg]

Looks good.

[jingham]

Thanks, that looks good to me too.

[bhushan]

Closed by commit rL246015

include/lldb/Core/Disassembler.h

Dump (Stream *s,
           const SymbolContext *sym_ctx,
           const SymbolContext *prev_sym_ctx,
           const FormatEntity::Entry *disassembly_addr_format,
           size_t max_address_text_size);
     
     virtual bool
     DoesBranch () = 0;
 
+    virtual bool
+    HasDelaySlot ();
+
     virtual size_t
     Decode (const Disassembler &disassembler, 
             const DataExtractor& data,
             lldb::offset_t data_offset) = 0;
             
     virtual void
     SetDescription (const char *) {}  // May be overridden in sub-classes that have descriptions.
     
public:
     PseudoInstruction ();
     
      virtual
      ~PseudoInstruction ();
      
     virtual bool
     DoesBranch ();
 
+    virtual bool
+    HasDelaySlot ();
+
     virtual void
     CalculateMnemonicOperandsAndComment (const ExecutionContext* exe_ctx)
     {
         // TODO: fill this in and put opcode name into Instruction::m_opcode_name,
         // mnemonic into Instruction::m_mnemonics, and any comment into 
         // Instruction::m_comment
     }
     

include/lldb/Target/Target.h

public:
     /// If \a addr_class is set to eAddressClassInvalid, then the address
     /// adjustment will always happen. If it is set to an address class
     /// that doesn't have code in it, LLDB_INVALID_ADDRESS will be 
     /// returned.
     //------------------------------------------------------------------
     lldb::addr_t
     GetOpcodeLoadAddress (lldb::addr_t load_addr, lldb::AddressClass addr_class = lldb::eAddressClassInvalid) const;
 
+    // Get load_addr as breakable load address for this target.
+    // Take a addr and check if for any reason there is a better address than this to put a breakpoint on.
+    // If there is then return that address.
+    // For MIPS, if instruction at addr is a delay slot instruction then this method will find the address of its
+    // previous instruction and return that address.
+    lldb::addr_t
+    GetBreakableLoadAddress (lldb::addr_t addr);
+
 protected:
     //------------------------------------------------------------------
     /// Implementing of ModuleList::Notifier.
     //------------------------------------------------------------------
     
     virtual void
     ModuleAdded (const ModuleList& module_list, const lldb::ModuleSP& module_sp);
     

source/Core/Disassembler.cpp

Instruction::DumpEmulation (const ArchSpec &arch)
   {
         insn_emulator_ap->SetInstruction (GetOpcode(), GetAddress(), NULL);
         return insn_emulator_ap->EvaluateInstruction (0);
   }
 
     return false;
 }
 
+bool
+Instruction::HasDelaySlot ()
+{
+    // Default is false.
+    return false;
+}
+
 OptionValueSP
 Instruction::ReadArray (FILE *in_file, Stream *out_stream, OptionValue::Type data_type)
 {
     bool done = false;
     char buffer[1024];
     
     OptionValueSP option_value_sp (new OptionValueArray (1u << data_type));
     

      
 bool
 PseudoInstruction::DoesBranch ()
 {
     // This is NOT a valid question for a pseudo instruction.
     return false;
 }
     
+bool
+PseudoInstruction::HasDelaySlot ()
+{
+    // This is NOT a valid question for a pseudo instruction.
+    return false;
+}
+
 size_t
 PseudoInstruction::Decode (const lldb_private::Disassembler &disassembler,
                            const lldb_private::DataExtractor &data,
                            lldb::offset_t data_offset)
 {
     return m_opcode.GetByteSize();
 }
 

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h

public:
         LLVMCDisassembler (const char *triple, const char *cpu, const char *features_str, unsigned flavor, DisassemblerLLVMC &owner);
 
         ~LLVMCDisassembler();
 
         uint64_t GetMCInst (const uint8_t *opcode_data, size_t opcode_data_len, lldb::addr_t pc, llvm::MCInst &mc_inst);
         uint64_t PrintMCInst (llvm::MCInst &mc_inst, char *output_buffer, size_t out_buffer_len);
         void     SetStyle (bool use_hex_immed, HexImmediateStyle hex_style);
         bool     CanBranch (llvm::MCInst &mc_inst);
+        bool     HasDelaySlot (llvm::MCInst &mc_inst);
         bool     IsValid()
         {
             return m_is_valid;
         }
 
     private:
         bool                                     m_is_valid;
         std::unique_ptr<llvm::MCContext>         m_context_ap;

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

 {
 public:
     InstructionLLVMC (DisassemblerLLVMC &disasm,
                       const lldb_private::Address &address,
                       AddressClass addr_class) :
         Instruction (address, addr_class),
         m_disasm_sp (disasm.shared_from_this()),
         m_does_branch (eLazyBoolCalculate),
+        m_has_delay_slot (eLazyBoolCalculate),
         m_is_valid (false),
         m_using_file_addr (false)
     {
     }
 
     virtual
     ~InstructionLLVMC ()
     {
DoesBranch ()
                         m_does_branch = eLazyBoolNo;
                 }
             }
             GetDisassemblerLLVMC().Unlock();
         }
         return m_does_branch == eLazyBoolYes;
     }
 
+    virtual bool
+    HasDelaySlot ()
+    {
+        if (m_has_delay_slot == eLazyBoolCalculate)
+        {
+            GetDisassemblerLLVMC().Lock(this, NULL);
+            DataExtractor data;
+            if (m_opcode.GetData(data))
+            {
+                bool is_alternate_isa;
+                lldb::addr_t pc = m_address.GetFileAddress();
+
+                DisassemblerLLVMC::LLVMCDisassembler *mc_disasm_ptr = GetDisasmToUse (is_alternate_isa);
+                const uint8_t *opcode_data = data.GetDataStart();
+                const size_t opcode_data_len = data.GetByteSize();
+                llvm::MCInst inst;
+                const size_t inst_size = mc_disasm_ptr->GetMCInst (opcode_data,
+                                                                   opcode_data_len,
+                                                                   pc,
+                                                                   inst);
+                // if we didn't understand the instruction, say it doesn't have a delay slot...
+                if (inst_size == 0)
+                    m_has_delay_slot = eLazyBoolNo;
+                else
+                {
+                    const bool has_delay_slot = mc_disasm_ptr->HasDelaySlot(inst);
+                    if (has_delay_slot)
+                        m_has_delay_slot = eLazyBoolYes;
+                    else
+                        m_has_delay_slot = eLazyBoolNo;
+                }
+            }
+            GetDisassemblerLLVMC().Unlock();
+        }
+        return m_has_delay_slot == eLazyBoolYes;
+    }
+
     DisassemblerLLVMC::LLVMCDisassembler *
     GetDisasmToUse (bool &is_alternate_isa)
     {
         is_alternate_isa = false;
         DisassemblerLLVMC &llvm_disasm = GetDisassemblerLLVMC();
         if (llvm_disasm.m_alternate_disasm_ap.get() != NULL)
         {
             const AddressClass address_class = GetAddressClass ();
public:
     GetDisassemblerLLVMC ()
     {
         return *(DisassemblerLLVMC *)m_disasm_sp.get();
     }
 protected:
 
     DisassemblerSP          m_disasm_sp; // for ownership
     LazyBool                m_does_branch;
+    LazyBool                m_has_delay_slot;
     bool                    m_is_valid;
     bool                    m_using_file_addr;
 };
 
 
 
 DisassemblerLLVMC::LLVMCDisassembler::LLVMCDisassembler (const char *triple, const char *cpu, const char *features_str, unsigned flavor, DisassemblerLLVMC &owner):
     m_is_valid(true)

 
 bool
 DisassemblerLLVMC::LLVMCDisassembler::CanBranch (llvm::MCInst &mc_inst)
 {
     return m_instr_info_ap->get(mc_inst.getOpcode()).mayAffectControlFlow(mc_inst, *m_reg_info_ap.get());
 }
 
 bool
+DisassemblerLLVMC::LLVMCDisassembler::HasDelaySlot (llvm::MCInst &mc_inst)
+{
+    return m_instr_info_ap->get(mc_inst.getOpcode()).hasDelaySlot();
+}
+
+bool
 DisassemblerLLVMC::FlavorValidForArchSpec (const lldb_private::ArchSpec &arch, const char *flavor)
 {
     llvm::Triple triple = arch.GetTriple();
     if (flavor == NULL || strcmp (flavor, "default") == 0)
         return true;
 
     if (triple.getArch() == llvm::Triple::x86 || triple.getArch() == llvm::Triple::x86_64)
     {

source/Target/Target.cpp

 #include "lldb/Host/Host.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionGroupWatchpoint.h"
 #include "lldb/Interpreter/OptionValues.h"
 #include "lldb/Interpreter/Property.h"
 #include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/Function.h"
+#include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/ObjCLanguageRuntime.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/SystemRuntime.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadSpec.h"
Target::CreateBreakpoint (const FileSpecList *containingModules,
     return CreateBreakpoint (filter_sp, resolver_sp, internal, hardware, true);
 }
 
 
 BreakpointSP
 Target::CreateBreakpoint (lldb::addr_t addr, bool internal, bool hardware)
 {
     Address so_addr;
+    
+    // Check for any reason we want to move this breakpoint to other address.
+    addr = GetBreakableLoadAddress(addr);
+
     // Attempt to resolve our load address if possible, though it is ok if
     // it doesn't resolve to section/offset.
 
     // Try and resolve as a load address if possible
     GetSectionLoadList().ResolveLoadAddress(addr, so_addr);
     if (!so_addr.IsValid())
     {
         // The address didn't resolve, so just set this as an absolute address
case llvm::Triple::thumb:
         break;
             
     default:
         break;
     }
     return opcode_addr;
 }
 
+lldb::addr_t
+Target::GetBreakableLoadAddress (lldb::addr_t addr)
+{
+    addr_t breakable_addr = addr;
+    Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_BREAKPOINTS));
+
+    switch (m_arch.GetMachine())
+    {
+    default:
+        break;
+    case llvm::Triple::mips:
+    case llvm::Triple::mipsel:
+    case llvm::Triple::mips64:
+    case llvm::Triple::mips64el:
+    {
+        addr_t function_start = 0;
+        addr_t current_offset = 0;
+        uint32_t loop_count = 0;
+        Address resolved_addr;
+        uint32_t arch_flags = m_arch.GetFlags ();
+        bool IsMips16 = arch_flags & ArchSpec::eMIPSAse_mips16;
+        bool IsMicromips = arch_flags & ArchSpec::eMIPSAse_micromips;
+        SectionLoadList &section_load_list = GetSectionLoadList();
+
+        if (section_load_list.IsEmpty())
+            // No sections are loaded, so we must assume we are not running yet
+            // and need to operate only on file address.
+            m_images.ResolveFileAddress (addr, resolved_addr); 
+        else
+            section_load_list.ResolveLoadAddress(addr, resolved_addr);
+
+        // Get the function boundaries to make sure we don't scan back before the beginning of the current function.
+        ModuleSP temp_addr_module_sp (resolved_addr.GetModule());
+        if (temp_addr_module_sp)
+        {
+            SymbolContext sc;
+            uint32_t resolve_scope = eSymbolContextFunction | eSymbolContextSymbol;
+            uint32_t resolved_mask = temp_addr_module_sp->ResolveSymbolContextForAddress(resolved_addr, resolve_scope, sc);
+            if (sc.function)
+            {
+                function_start = sc.function->GetAddressRange().GetBaseAddress().GetLoadAddress(this);
+                if (function_start == LLDB_INVALID_ADDRESS)
+                    function_start = sc.function->GetAddressRange().GetBaseAddress().GetFileAddress();
+            }
+            else if (sc.symbol)
+            {
+                Address sym_addr = sc.symbol->GetAddress();
+                function_start = sym_addr.GetFileAddress();
+            }
+            current_offset = addr - function_start;
+        }
+
+        // If breakpoint address is start of function then we dont have to do anything.
+        if (current_offset == 0)
+            return breakable_addr;
+        else
+            loop_count = current_offset / 2;
+
+        if (loop_count > 3)
+        {
+            // Scan previous 6 bytes
+            if (IsMips16 | IsMicromips)
+                loop_count = 3;
+            // For mips-only, instructions are always 4 bytes, so scan previous 4 bytes only.
+            else
+                loop_count = 2;
+        }
+
+        // Create Disassembler Instance
+        lldb::DisassemblerSP disasm_sp (Disassembler::FindPlugin(m_arch, NULL, NULL));
+
+        ExecutionContext exe_ctx;
+        CalculateExecutionContext(exe_ctx);
+        InstructionList instruction_list;
+        InstructionSP prev_insn;
+        bool prefer_file_cache = true; // Read from file
+        uint32_t inst_to_choose = 0;
+
+        for (uint32_t i = 1; i <= loop_count; i++)
+        {
+            // Adjust the address to read from.
+            resolved_addr.Slide (-2);
+            AddressRange range(resolved_addr, i*2);
+            uint32_t insn_size = 0;
+
+            uint32_t bytes_disassembled = disasm_sp->ParseInstructions (&exe_ctx, range, NULL, prefer_file_cache);
+            
+            uint32_t num_insns = disasm_sp->GetInstructionList().GetSize();
+            if (num_insns)
+            {
+                prev_insn = disasm_sp->GetInstructionList().GetInstructionAtIndex(0);
+                insn_size = prev_insn->GetOpcode().GetByteSize();
+                if (i == 1 && insn_size == 2)
+                {
+                    // This looks like a valid 2-byte instruction (but it could be a part of upper 4 byte instruction).
+                    instruction_list.Append(prev_insn);
+                    inst_to_choose = 1;
+                }
+                else if (i == 2)
+                {
+                    // Here we may get one 4-byte instruction or two 2-byte instructions.
+                    if (num_insns == 2)
+                    {
+                        // Looks like there are two 2-byte instructions above our breakpoint target address.
+                        // Now the upper 2-byte instruction is either a valid 2-byte instruction or could be a part of it's upper 4-byte instruction.
+                        // In both cases we don't care because in this case lower 2-byte instruction is definitely a valid instruction
+                        // and whatever i=1 iteration has found out is true.
+                        inst_to_choose = 1;
+                        break;
+                    }
+                    else if (insn_size == 4)
+                    {
+                        // This instruction claims its a valid 4-byte instruction. But it could be a part of it's upper 4-byte instruction.
+                        // Lets try scanning upper 2 bytes to verify this.
+                        instruction_list.Append(prev_insn);
+                        inst_to_choose = 2;
+                    }
+                }
+                else if (i == 3)
+                {
+                    if (insn_size == 4)
+                        // FIXME: We reached here that means instruction at [target - 4] has already claimed to be a 4-byte instruction,
+                        // and now instruction at [target - 6] is also claiming that it's a 4-byte instruction. This can not be true.
+                        // In this case we can not decide the valid previous instruction so we let lldb set the breakpoint at the address given by user.
+                        inst_to_choose = 0;
+                    else
+                        // This is straight-forward 
+                        inst_to_choose = 2;
+                    break;
+                }
+            }
+            else
+            {
+                // Decode failed, bytes do not form a valid instruction. So whatever previous iteration has found out is true.
+                if (i > 1)
+                {
+                    inst_to_choose = i - 1;
+                    break;
+                }
+            }
+        }
+
+        // Check if we are able to find any valid instruction.
+        if (inst_to_choose)
+        {
+            if (inst_to_choose > instruction_list.GetSize())
+                inst_to_choose--;
+            prev_insn = instruction_list.GetInstructionAtIndex(inst_to_choose - 1);
+
+            if (prev_insn->HasDelaySlot())
+            {
+                uint32_t shift_size = prev_insn->GetOpcode().GetByteSize();
+                // Adjust the breakable address
+                breakable_addr = addr - shift_size;
+                if (log)
+                    log->Printf ("Target::%s Breakpoint at 0x%8.8" PRIx64 " is adjusted to 0x%8.8" PRIx64 " due to delay slot\n", __FUNCTION__, addr, breakable_addr);
+            }
+        }
+        break;
+    }
+    }
+    return breakable_addr;
+}
+
 SourceManager &
 Target::GetSourceManager ()
 {
     if (m_source_manager_ap.get() == NULL)
         m_source_manager_ap.reset (new SourceManager(shared_from_this()));
     return *m_source_manager_ap;
 }