[Lldb-commits] [PATCH] D10954: [LLDB][MIPS] Added test case support for MIPS

[Greg Clayton]

clayborg requested changes to this revision.

This revision now requires changes to proceed.

inline comments above


================
Comment at: test/python_api/value/change_values/TestChangeValueAPI.py:146-149
@@ -144,1 +145,6 @@
+        #To get invalid  address in case of MIPS Architecture when sp value is changed. 
+        if arch == "mips" or arch == "mips64" or arch == "mips32el" or arch == "mips64el":
+           result = sp_value.SetValueFromCString("100")
+        else:
+           result = sp_value.SetValueFromCString("1")
         self.assertTrue (result, "Setting sp returned true.")
----------------
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?

================
Comment at: test/tools/lldb-server/gdbremote_testcase.py:1261-1266
@@ -1260,2 +1260,8 @@
         self.assertTrue(state_reached)
+        expected_step_count = 1
+        arch = self.getArchitecture()
+
+        #MIPS required "3" (ADDIU, SB, LD) machine instructions for updation of variable value
+        if arch == "mips" or arch == "mips64" or arch == "mips32el" or arch == "mips64el":
+           expected_step_count = 3
 
----------------
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


Repository:
  rL LLVM

http://reviews.llvm.org/D10954