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–363

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().

382–404

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

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

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

1098

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

383–385

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

[clayborg]

More inline comments.

include/lldb/Core/Disassembler.h

228

Pass a Target instead?

source/Core/Disassembler.cpp

1064

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/Core/Disassembler.h

public:
     
     uint32_t
     GetMaxOpcocdeByteSize () const;
 
     lldb::InstructionSP
     GetInstructionAtIndex (size_t idx) const;
     
     uint32_t
-    GetIndexOfNextBranchInstruction(uint32_t start) const;
+    GetIndexOfNextBranchInstruction(uint32_t start, Target &target) const;
     
     uint32_t
     GetIndexOfInstructionAtLoadAddress (lldb::addr_t load_addr, Target &target);
 
     uint32_t
     GetIndexOfInstructionAtAddress (const Address &addr);
 
     void

source/Core/Disassembler.cpp

 void
 InstructionList::Append (lldb::InstructionSP &inst_sp)
 {
     if (inst_sp)
         m_instructions.push_back(inst_sp);
 }
 
 uint32_t
-InstructionList::GetIndexOfNextBranchInstruction(uint32_t start) const
+InstructionList::GetIndexOfNextBranchInstruction(uint32_t start, Target &target) const
 {
     size_t num_instructions = m_instructions.size();
     
     uint32_t next_branch = UINT32_MAX;
-    for (size_t i = start; i < num_instructions; i++)
+    size_t i;
+    for (i = start; i < num_instructions; i++)
     {
         if (m_instructions[i]->DoesBranch())
         {
             next_branch = i;
             break;
         }
     }
+
+    // Hexagon needs the first instruction of the packet with the branch.
+    // Go backwards until we find an instruction marked end-of-packet, or
+    // until we hit start.
+    if (target.GetArchitecture().GetTriple().getArch() == llvm::Triple::hexagon)
+    {
+        // If we didn't find a branch, find the last packet start.
+        if (next_branch == UINT32_MAX)
+        {
+            i = num_instructions - 1;
+        }
+
+        while (i > start)
+        {
+            --i;
+
+            Error error;
+            uint32_t inst_bytes;
+            bool prefer_file_cache = false; // Read from process if process is running
+            lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
+            target.ReadMemory(m_instructions[i]->GetAddress(),
+                              prefer_file_cache,
+                              &inst_bytes,
+                              sizeof(inst_bytes),
+                              error,
+                              &load_addr);
+            // If we have an error reading memory, return start
+            if (!error.Success())
+                return start;
+            // 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
+                next_branch = i + 1;
+                break;
+            }
+        }
+
+        if (next_branch == UINT32_MAX)
+        {
+            // We couldn't find the previous packet, so return start
+            next_branch = start;
+        }
+    }
     return next_branch;
 }
 
 uint32_t
 InstructionList::GetIndexOfInstructionAtAddress (const Address &address)
 {
     size_t num_instructions = m_instructions.size();
     uint32_t index = UINT32_MAX;

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::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;
     else
     {
+        Target &target = GetThread().GetProcess()->GetTarget();
         uint32_t branch_index;
-        branch_index = instructions->GetIndexOfNextBranchInstruction (pc_index);
+        branch_index = instructions->GetIndexOfNextBranchInstruction (pc_index, target);
         
         Address run_to_address;
         
         // 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();
                         }