This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][LoongArch] Make software single stepping work
ClosedPublic

Authored by lh03061238 on Dec 1 2022, 5:04 PM.

Details

Summary

Hardware single stepping is not currently supported by the linux kernel.
In order to support single step debugging, add EmulateInstructionLoongArch
to implement the software Single Stepping. This patch only support the
simplest single step execution of non-jump instructions.

Diff Detail

Event Timeline

lh03061238 created this revision.Dec 1 2022, 5:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 5:04 PM
lh03061238 requested review of this revision.Dec 1 2022, 5:04 PM
jingham added a subscriber: jingham.Dec 1 2022, 6:05 PM

Note that the two most common uses of single step in lldb are 1) stepping over the instruction under a breakpoint (which may be a branch) and 2) stepping to the next instruction from a branch instruction. So stepping won't work correctly till you get single step past a branch instruction working. But you probably knew that...

Note that the two most common uses of single step in lldb are 1) stepping over the instruction under a breakpoint (which may be a branch) and 2) stepping to the next instruction from a branch instruction. So stepping won't work correctly till you get single step past a branch instruction working. But you probably knew that...

Yes, you're right.
This patch just make the single stepping to work, only the non-jump instructions are supported, the effect is just like pc + 4. Support for jump instructions will be added in next patch.

I'm not sure whether lldb should follow llvm coding standard.

lldb/source/Plugins/Instruction/LoongArch/CMakeLists.txt
8–9

It's better to sort alphabetically.

lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp
38

Will the mask be changed in future? If so, better to leave a FIXME or TODO. If not, the following for loop always return the NonJMP opcode?

40

loongarch?

42

useless {

45

Ditto.

50
67–69
if (!(this->*opcode_data->callback)(inst))
  return false;
76–79
if (new_pc == old_pc && !WritePC(old_pc + inst_size)
    return false;
85

Remove useless blank line.

175

useless {

177

Ditto

lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
29–37

Do you plan to support eInstructionTypeAny eInstructionTypePrologueEpilogue eInstructionTypeAll in future? If so, had better leave a TODO here. Otherwise this function can be simplied to:

return inst_type == eInstructionTypePCModifying;
67

Could it be a private function?

80

Is it a override function?

lldb/tools/lldb-server/SystemInitializerLLGS.cpp
56

Sort alphabetically?

xen0n added a comment.Dec 4 2022, 9:52 AM

I think you can remove the example from your commit message altogether, it's giving essentially no information while making the text very lengthy. Only keeping the first paragraph should be enough.

lldb/source/Plugins/Instruction/CMakeLists.txt
8

The list is sorted alphabetically so this should come before MIPS.

lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
11

Use ALL_CAPS for this include guard symbol.

25

Use "LoongArch" here because it's intended to be a human-readable string.

lldb/tools/lldb-server/CMakeLists.txt
58

Put before MIPS for alphabetical order too.

lldb/tools/lldb-server/SystemInitializerLLGS.cpp
54

There's no __loongarch, only __loongarch__ (we're more classical than RISC-V in this regard). Instead simply use #if defined(__loongarch__) as the code enabled by this condition isn't really caring its bitness.

77–79

Alphabetize this chunk too.

99–101

Ditto.

lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
67

Could it be a private function?

We want this function to be used in its subclass,
so we define it as public

80

Is it a override function?

It is not override function, this function simply returns false.
SetupSoftwareSingleStepping () function will be updated PC (next_pc = PC + 4), when EvaluateInstruction exit.

lh03061238 updated this revision to Diff 480297.Dec 5 2022, 5:51 PM
lh03061238 edited the summary of this revision. (Show Details)

(1) Use sort alphabetically in the following file
lldb/source/Plugins/Instruction/CMakeLists.txt
lldb/source/Plugins/Instruction/LoongArch/CMakeLists.txt
lldb/tools/lldb-server/CMakeLists.txt
lldb/tools/lldb-server/SystemInitializerLLGS.cpp

(2) Use ALL_CAPS for include guard symbol

(3) Modify the following functions
SupportsThisInstructionType(InstructionType inst_type)
GetOpcodeForInstruction(uint32_t inst)
EvaluateInstruction(uint32_t options)
CreateInstance(const ArchSpec &arch,InstructionType inst_type)

(4) Added TODO description in GetOpcodeForInstruction(uint32_t inst)

(5) Use #if defined(loongarch) in lldb/tools/lldb-server/SystemInitializerLLGS.cpp

(6) Remove the example from commit message

Looks mechanically fine. These classes are all a bit of a copy paste job right now, so if you were going to take inspiration riscv is your best bet.

On the subject of coding style, https://lldb.llvm.org/resources/contributing.html.

Coding Style: LLDB’s code style differs from LLVM’s coding style. Unfortunately there is no document describing the differences. Please be consistent with the existing code.

Which isn't super helpful but in general look at something central like lldb/source/Target/Process.cpp and see what it does. The main difference you'll see is variable_names_with_underscores_like_this and m_ prefix for class members.

For me, the style here looks fine.

lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
59

I think the older targets use this form but for riscv they went with llvm::Optional<lldb::addr_t> ReadPC(); which I prefer over pointer plus addr_t.

SixWeining accepted this revision.Dec 6 2022, 5:44 PM

Looks mechanically fine. These classes are all a bit of a copy paste job right now, so if you were going to take inspiration riscv is your best bet.

On the subject of coding style, https://lldb.llvm.org/resources/contributing.html.

Coding Style: LLDB’s code style differs from LLVM’s coding style. Unfortunately there is no document describing the differences. Please be consistent with the existing code.

Which isn't super helpful but in general look at something central like lldb/source/Target/Process.cpp and see what it does. The main difference you'll see is variable_names_with_underscores_like_this and m_ prefix for class members.

For me, the style here looks fine.

Thanks for the clarification. Then LGTM for LoongArch related change.

This revision is now accepted and ready to land.Dec 6 2022, 5:44 PM

Looks mechanically fine. These classes are all a bit of a copy paste job right now, so if you were going to take inspiration riscv is your best bet.

On the subject of coding style, https://lldb.llvm.org/resources/contributing.html.

Coding Style: LLDB’s code style differs from LLVM’s coding style. Unfortunately there is no document describing the differences. Please be consistent with the existing code.

Which isn't super helpful but in general look at something central like lldb/source/Target/Process.cpp and see what it does. The main difference you'll see is variable_names_with_underscores_like_this and m_ prefix for class members.

For me, the style here looks fine.

Thanks for your suggestion

lh03061238 added inline comments.Dec 6 2022, 6:48 PM
lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
59

I think the older targets use this form but for riscv they went with llvm::Optional<lldb::addr_t> ReadPC(); which I prefer over pointer plus addr_t.

EmulateInstructionLoongArch is relatively simple Compared with riscv. If use llvm::Optional<lldb::addr_t> ReadPC(), There will be more type
conversions here. I prefer to keep that definition for now, Considering the complexity of the code at this stage.

Thank you.

seehearfeel added inline comments.Dec 7 2022, 11:14 PM
lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
59

@DavidSpickett Are you OK with the current code?

DavidSpickett accepted this revision.Dec 8 2022, 2:45 AM

This LGTM.

lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
59

I prefer to keep that definition for now, Considering the complexity of the code at this stage.

Totally fine, this is a lot of duplicating existing code to begin with so I appreciate why you'd keep the differences minimal.

This revision was automatically updated to reflect the committed changes.