This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][LoongArch] Add unittests for EmulateInstructionLoongArch
ClosedPublic

Authored by lh03061238 on Dec 20 2022, 3:58 AM.

Details

Summary

Add unit tests For EmulateInstructionLoongArch existing branch instruction.
Add 19 test cases in total.

Without this patch:

$ ninja check-lldb-unit
[0/1] Running lldb unit test suite

Testing Time: 10.55s
  Passed: 1025

With this patch:

$ ninja check-lldb-unit
[0/1] Running lldb unit test suite

Testing Time: 10.45s
  Passed: 1044

Diff Detail

Event Timeline

lh03061238 created this revision.Dec 20 2022, 3:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2022, 3:58 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
lh03061238 requested review of this revision.Dec 20 2022, 3:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2022, 3:58 AM

Please split this patch into 2:

  • the cleanup of the existing branch instructions
  • the addition of the new ones

The changes look good but let's keep each commit to doing 1 thing.

You are missing tests though, and this is my mistake for not asking for them in the previous patches. See https://reviews.llvm.org/D139294 for examples of how riscv is doing it. It doesn't need to be totally comprehensive. For example we can assume basic register operands are extracted correctly. So for a branch the tests would be one where it is supposed to branch and one where it isn't. The infrastructure for testing the emulation varies between arch but as before, riscv probably the best reference point.

Please add testing for the existing instructions as parent patches of this one (these 2 once you've split them), then these new ones should have tests too.

Note: I will be off for Christmas break from tomorrow until the 2nd week of January. So you can add any of the other common reviewers in lldb in the meantime. Or wait for me to be back, feel free to pile up reviews :)

Please split this patch into 2:

  • the cleanup of the existing branch instructions
  • the addition of the new ones

The changes look good but let's keep each commit to doing 1 thing.

You are missing tests though, and this is my mistake for not asking for them in the previous patches. See https://reviews.llvm.org/D139294 for examples of how riscv is doing it. It doesn't need to be totally comprehensive. For example we can assume basic register operands are extracted correctly. So for a branch the tests would be one where it is supposed to branch and one where it isn't. The infrastructure for testing the emulation varies between arch but as before, riscv probably the best reference point.

Please add testing for the existing instructions as parent patches of this one (these 2 once you've split them), then these new ones should have tests too.

Note: I will be off for Christmas break from tomorrow until the 2nd week of January. So you can add any of the other common reviewers in lldb in the meantime. Or wait for me to be back, feel free to pile up reviews :)

Thanks,

I will modify the content of this patch to add unit tests first.

lh03061238 retitled this revision from [LLDB][LoongArch] Add FP branch instructions for EmulateInstructionLoongArch to [LLDB][LoongArch] Add unittests for EmulateInstructionLoongArch.
lh03061238 edited the summary of this revision. (Show Details)

Add unittests For EmulateInstructionLoongArch existing branch instruction.

The tests LGTM.

lldb/unittests/Instruction/LoongArch/TestLoongArchEmulator.cpp
2–3

Merge these lines into one.

Modify the first two lines in TestLoongArchEmulator.cpp

lh03061238 added inline comments.Dec 29 2022, 12:52 AM
lldb/unittests/Instruction/LoongArch/TestLoongArchEmulator.cpp
2–3

It has been modified, thanks.

This revision is now accepted and ready to land.Jan 12 2023, 2:29 AM