Add Hexagon packet support to ThreadPlanStepRange

[ted]

Summary

Hexagon is a VLIW processor. It can execute multiple instructions at once, called a packet. Breakpoints need to be alone in a packet. This patch will make sure that temporary breakpoints used for stepping are set at the start of a packet, which will put the breakpoint in a packet by itself.

Patch by Deepak Panickal of CodePlay and Ted Woodward of Qualcomm.

[clayborg]

See inline comments for changes.

source/Target/ThreadPlanStepRange.cpp

362–411

This should probably be moved into

uint32_t InstructionList::GetIndexOfNextBranchInstruction(uint32_t start) const;

And the GetIndexOfNextBranchInstruction should probably take a new argument: "const ArchSpec &arch" so it can do the right thing. We would want your code above to not happen for any other clients of InstructionList::GetIndexOfNextBranchInstruction().

433–455

This code should probably become:

uint32_t branch_index;
branch_index = instructions->GetIndexOfNextBranchInstruction(m_thread.GetProcess()->GetTarget().GetArchitecture() , pc_index);

See my previous inline comment.

[ted]

Code moved to GetIndexOfNextBranchInstruction(), as requested.

I pass in a ProcessSP instead of an ArchSpec because I need to read memory. Hexagon specific code gated by a check to make sure the Triple contains llvm::Triple::hexagon.

[clayborg]

Looks better. You might consider using Target::ReadMemory, see inline comments for the explanation of why.

include/lldb/Core/Disassembler.h

228 ↗

(On Diff #25101)

Make this a "const lldb::ProcessSP &process_sp" so we can call this using a temporary like:

foo-> GetIndexOfNextBranchInstruction(12, object->GetProcess())

source/Core/Disassembler.cpp

1064 ↗

(On Diff #25101)

Make this a "const lldb::ProcessSP &process_sp"

1098 ↗

(On Diff #25101)

You might want to pass in a target and use Target::ReadMemory() and prefer to use:

bool prefer_file_cache = false; // Read from process if the process is running, else fall back to file cache
lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
size_t bytes_read = target->ReadMemory(m_instructions[i]->GetAddress(), prefer_file_cache, &inst_bytes, sizeof(inst_bytes), error, &load_addr);

As this could allow you to determine next branch instruction when not running. Target::ReadMemory() can read from the .text section of a file (the file cache as mentioned in the "prefer_file_cache" variable) if you aren't connected to a running process. And opcodes don't often change just because you are running.

source/Target/ThreadPlanStepRange.cpp

448–450

Pass the target to instructions->GetIndexOfNextBranchInstruction() instead of the process?

[clayborg]

More inline comments.

include/lldb/Core/Disassembler.h

228 ↗

(On Diff #25101)

Pass a Target instead?

source/Core/Disassembler.cpp

1064 ↗

(On Diff #25101)

Pass a target instead?

[ted]

Changed parameter from ProcessSP to Target, as requested.

[clayborg]

Looks good except I would change it to not require a target (Target &) and but take a Target * so people don't have to pass in a target.

[ted]

InstructionList::GetIndexOfNextBranchInstruction() is only called from one spot, ThreadPlanStepRange::SetNextBranchBreakpoint(), and it needs a Target to get the the ArchSpec to check for Hexagon. I could change it to only do the check if the Target wasn't nullptr, but that seems unnecessary to me.

[clayborg]

That's fine. We can change this later if ever needed.

[jingham]

Why is this something we only have to do when setting the "next branch breakpoint"? Shouldn't this happen every time we go to write a breakpoint?

[ted]

That's a good point - breakpoints set on functions or source lines will get the start of a packet, but if the user sets a breakpoint on an address we should fix that to set it at the start of the packet containing that address.

Where should I make that change?

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[jingham]

In D9437#170505, @ted wrote:

That's a good point - breakpoints set on functions or source lines will get the start of a packet, but if the user sets a breakpoint on an address we should fix that to set it at the start of the packet containing that address.

Where should I make that change?

Probably where the breakpoint site gets made. But this might get tricky. What happens if the user sets breakpoints on two addresses in the same "packet"? Do you need to move them both to the beginning of the packet? And if so, how will the breakpoint system tell which breakpoint functionally got hit?

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

include/lldb/Target/ThreadPlanStepRange.h

protected:
     GetInstructionsForAddress(lldb::addr_t addr, size_t &range_index, size_t &insn_offset);
     
     // Pushes a plan to proceed through the next section of instructions in the range - usually just a RunToAddress
     // plan to run to the next branch.  Returns true if it pushed such a plan.  If there was no available 'quick run'
     // plan, then just single step.
     bool
     SetNextBranchBreakpoint ();
     
+    // packet aware branch index finding strategy for hexagon architecture
+    bool
+    GetBranchIndexForHexagon( const InstructionList * instructions,
+                              const uint32_t pc_index,
+                              uint32_t & branch_index);
+
     void
     ClearNextBranchBreakpoint();
     
     bool
     NextRangeBreakpointExplainsStop (lldb::StopInfoSP stop_info_sp);
     
     SymbolContext             m_addr_context;
     std::vector<AddressRange> m_address_ranges;

source/Target/ThreadPlanStepRange.cpp

if (m_next_branch_bp_sp)
         Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_STEP));
         if (log)
             log->Printf ("Removing next branch breakpoint: %d.", m_next_branch_bp_sp->GetID());
         GetTarget().RemoveBreakpointByID (m_next_branch_bp_sp->GetID());
         m_next_branch_bp_sp.reset();
     }
 }
 
 bool
+ThreadPlanStepRange::GetBranchIndexForHexagon( const InstructionList * instructions,
+                                               const uint32_t pc_index,
+                                               uint32_t & branch_index)
+{
+    Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
+
+    branch_index = pc_index;
+
+    // find the farthest viable packet within the range
+    {
+        Error error;
+
+                // read all instructions up to the end of the range (not the last instruction!!)
+        for (uint32_t ix = pc_index; ix < instructions->GetSize() - 1; ix++)
+        {
+            lldb::InstructionSP inst = instructions->GetInstructionAtIndex(ix);
+            // if this instruction branched then we can only jump as far as branch_index
+            // which should be the last start of a packet we saw
+            if (inst->DoesBranch())
+                break;
+
+            // read instruction from memory
+            uint32_t inst_bytes = 0;
+            Target & target = m_thread.GetProcess()->GetTarget();
+            lldb::addr_t addr = inst->GetAddress().GetLoadAddress(&target);
+            m_thread.GetProcess()->ReadMemory(addr, &inst_bytes, sizeof(inst_bytes), error);
+            if (!error.Success())
+                return false;
+
+            if (log)
+                log->Printf("Instruction @0x%08X : 0x%08X\n", (uint32_t)addr, inst_bytes);
+
+            // check if this is the last instruction in a packet
+            // bits 15:14 will be 11b or 00b for a duplex
+            if (((inst_bytes & 0xC000) == 0xC000) ||
+                ((inst_bytes & 0xC000) == 0x0000))
+            {
+                // instruction after this should be the start of next packet
+                branch_index = ix + 1;
+
+                if (log)
+                    log->Printf("Packet start @0x%08X\n", (uint32_t)addr + 4);
+            }
+        }
+    }
+
+    // at this step branch_index should be packet aligned
+    return true;
+}
+
+bool
 ThreadPlanStepRange::SetNextBranchBreakpoint ()
 {
     if (m_next_branch_bp_sp)
         return true;
 
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_STEP));
     // Stepping through ranges using breakpoints doesn't work yet, but with this off we fall back to instruction
     // single stepping.
     if (!m_use_fast_step)
          return false;
 
     lldb::addr_t cur_addr = GetThread().GetRegisterContext()->GetPC();
     // Find the current address in our address ranges, and fetch the disassembly if we haven't already:
     size_t pc_index;
     size_t range_index;
     InstructionList *instructions = GetInstructionsForAddress (cur_addr, range_index, pc_index);
     if (instructions == NULL)
         return false;
+
+    uint32_t branch_index = pc_index;
+
+    // packet aware stepping strategy for hexagon architecture
+    const llvm::Triple &triple_ref = m_thread.GetProcess()->GetTarget().GetArchitecture().GetTriple();
+    if (triple_ref.getArch() == llvm::Triple::hexagon)
+    {
+        if (log)
+            log->Printf("Finding branch index from address @ 0x%08x", (uint32_t)cur_addr);
+
+        // get a packet aligned branch index
+        if (!GetBranchIndexForHexagon(instructions, pc_index, branch_index))
+            return false;
+    }
     else
     {
-        uint32_t branch_index;
-        branch_index = instructions->GetIndexOfNextBranchInstruction (pc_index);
-        
-        Address run_to_address;
-        
+        branch_index = instructions->GetIndexOfNextBranchInstruction(pc_index);
         // If we didn't find a branch, run to the end of the range.
         if (branch_index == UINT32_MAX)
         {
             branch_index = instructions->GetSize() - 1;
         }
-        
-        if (branch_index - pc_index > 1)
-        {
-            const bool is_internal = true;
-            run_to_address = instructions->GetInstructionAtIndex(branch_index)->GetAddress();
-            m_next_branch_bp_sp = GetTarget().CreateBreakpoint(run_to_address, is_internal, false);
-            if (m_next_branch_bp_sp)
-            {
-                if (log)
-                {
-                    lldb::break_id_t bp_site_id = LLDB_INVALID_BREAK_ID;
-                    BreakpointLocationSP bp_loc = m_next_branch_bp_sp->GetLocationAtIndex(0);
-                    if (bp_loc)
-                    {
-                        BreakpointSiteSP bp_site = bp_loc->GetBreakpointSite();
-                        if (bp_site)
-                        {
-                            bp_site_id = bp_site->GetID();
-                        }
-                    }
-                    log->Printf ("ThreadPlanStepRange::SetNextBranchBreakpoint - Setting breakpoint %d (site %d) to run to address 0x%" PRIx64,
-                                 m_next_branch_bp_sp->GetID(),
-                                 bp_site_id,
-                                 run_to_address.GetLoadAddress(&m_thread.GetProcess()->GetTarget()));
-                }
-                m_next_branch_bp_sp->SetThreadID(m_thread.GetID());
-                m_next_branch_bp_sp->SetBreakpointKind ("next-branch-location");
-                return true;
-            }
-            else
-                return false;
-        }
+    }
+        Address run_to_address;
     }
     return false;
 }
 
 bool
 ThreadPlanStepRange::NextRangeBreakpointExplainsStop (lldb::StopInfoSP stop_info_sp)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_STEP));