Page MenuHomePhabricator

[LLDB][MIPS] Added test case support for MIPS
AbandonedPublic

Authored by nitesh.jain on Jul 6 2015, 4:02 AM.

Details

Reviewers
ovyalov
clayborg

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain updated this revision to Diff 29073.Jul 6 2015, 4:02 AM
nitesh.jain retitled this revision from to [LLDB][MIPS] Added test case support for MIPS.
nitesh.jain updated this object.
nitesh.jain added reviewers: clayborg, ovyalov.
nitesh.jain set the repository for this revision to rL LLVM.
emaste added a subscriber: emaste.Jul 6 2015, 6:24 AM
emaste added inline comments.
test/python_api/value/change_values/TestChangeValueAPI.py
145

Can you expand on this comment?

clayborg requested changes to this revision.Jul 6 2015, 10:10 AM
clayborg edited edge metadata.

inline comments above

test/python_api/value/change_values/TestChangeValueAPI.py
146–149

Can se just pick a safer SP value that would work on all systems? Maybe 32 or 64? Some systems might require SP to be aligned to a specific 4, 8 or 16 byte boundary and I am guessing that is what this change is trying to fix. Can we just change the SP value to 64?

test/tools/lldb-server/gdbremote_testcase.py
1261–1266

Not sure why we have a test that is counting single steps of instructions? Of course they are going to vary. Maybe you can check with the original author and ask why we are doing this? This doesn't seem portable or like a very good thing to test? I would rather see breakpoints being set at source lines and stop before and after and verify that things changed than to have any tests that expect stepping to work in a specific way. All of the different architectures (arm, arm64, i386, x86_64, mips, mips64, hexagon, etc) have front/back ends that produce different DWARF line tables so even trying to make a test that does something like:

  • set breakpoint 1 by file + line
  • run to breakpoint 1
  • test some initial condition
  • step over
  • step over
  • test something

are doomed to fail. We should be using breakpoints on file + line in order to test things so they work on all architectures and then the test should look like:

  • set breakpoint 1 by file + line
  • set breakpoint 2 by file + line
  • run to breakpoint 1
  • test some initial condition
  • run to breakpoint 2
  • test something
This revision now requires changes to proceed.Jul 6 2015, 10:10 AM
clayborg added inline comments.Jul 6 2015, 10:10 AM
test/python_api/value/change_values/TestChangeValueAPI.py
146–149

If we can change this to be 64 then we can remove the arch tests...

labath added a subscriber: labath.Jul 7 2015, 1:35 AM
labath added inline comments.
test/tools/lldb-server/gdbremote_testcase.py
1261–1266

As I understand it, these tests are supposed to verify behaviour of the lldb-server, sans lldb. This means they talk raw gdb-remote protocol over sockets and they don't have access to lldb functionality like parsing dwarf line tables.
That said, I don't have an idea how to make this more robust... Perhaps we don't need to care about the exact number of steps - it should be enough to verify that the state is reached within a reasonable number of steps (?)

tberghammer added inline comments.
test/tools/lldb-server/gdbremote_testcase.py
1261–1266

Based on my understanding these tests try to verify that the "s" (or the "vCont:s") packet only steps 1 instruction. For architectures where the instruction size is fixed we can compare the PC reported before the step and after the step but it won't work on x86 or with thumb

Ah, thanks for the info. Feel free to fix the gdbremote_testcase.py however you have to. My comments stand for the SP value in TestChangeValueAPI.py.

nitesh.jain marked an inline comment as done.Jul 13 2015, 11:37 PM
nitesh.jain added inline comments.
test/python_api/value/change_values/TestChangeValueAPI.py
145

When sp=1 , for MIPS64 it generate 64 bit invalid address causing process to exit. The solution for above problem was to set sp value greater than stack allocated size in Assembly Prologue. But it will vary from process to process hence not suitable approached. The good approach was to handle SIGBUS signal with si_code=SI_KERENEL for invalid 64 bit address.

The Patch [ http://reviews.llvm.org/D11176 | To handle SI_KERENEL generated for invalid 64 bit address ] handle above mentioned case

nitesh.jain added inline comments.Jul 13 2015, 11:44 PM
test/python_api/value/change_values/TestChangeValueAPI.py
146–149

Even changing sp to 64 couldn't cause the test case to passed. Since it was generating SIGBUS signal with si_code=SI_KERENEL for invalid 64 bit address. which was not handle.

The Patch [ http://reviews.llvm.org/D11176 | To handle SI_KERENEL generated for invalid 64 bit address ] handle above mentioned case.

clayborg accepted this revision.Jul 15 2015, 10:31 AM
clayborg edited edge metadata.

ok, it you need to special case for MIPS then you can do so.

This revision is now accepted and ready to land.Jul 15 2015, 10:31 AM
nitesh.jain abandoned this revision.Mar 23 2016, 6:42 AM