This is an archive of the discontinued LLVM Phabricator instance.

Add Hexagon packet support to ThreadPlanStepRange
ClosedPublic

Authored by ted on May 1 2015, 9:35 AM.

Details

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.

Diff Detail

Event Timeline

ted updated this revision to Diff 24813.May 1 2015, 9:35 AM
ted retitled this revision from to Add Hexagon packet support to ThreadPlanStepRange.
ted updated this object.
ted edited the test plan for this revision. (Show Details)
ted added reviewers: clayborg, deepak2427.
ted added a subscriber: Unknown Object (MLST).
clayborg requested changes to this revision.May 1 2015, 11:07 AM
clayborg edited edge metadata.

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.

This revision now requires changes to proceed.May 1 2015, 11:07 AM
ted updated this revision to Diff 25101.May 6 2015, 4:05 PM
ted edited edge metadata.

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 requested changes to this revision.May 6 2015, 5:04 PM
clayborg edited edge metadata.

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?

This revision now requires changes to proceed.May 6 2015, 5:04 PM

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 updated this revision to Diff 25239.EditedMay 7 2015, 2:45 PM
ted edited edge metadata.

Changed parameter from ProcessSP to Target, as requested.

clayborg requested changes to this revision.May 7 2015, 3:07 PM
clayborg edited edge metadata.

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.

This revision now requires changes to proceed.May 7 2015, 3:07 PM
ted added a comment.May 7 2015, 3:27 PM

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 accepted this revision.May 8 2015, 10:58 AM
clayborg edited edge metadata.

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

This revision is now accepted and ready to land.May 8 2015, 10:58 AM
ted closed this revision.May 8 2015, 12:14 PM

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 added a comment.May 11 2015, 2:54 PM

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

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