[Lldb-commits] [PATCH] D24187: Intel(R) Memory Protection Extensions (Intel(R) MPX) support.

[Valentina Giusti via lldb-commits]

valentinagiusti added a comment.

Thanks for the review! You can find my replies inline.


================
Comment at: packages/Python/lldbsuite/test/functionalities/register/intel_xtended_registers/TestMPXRegisters.py:27
@@ +26,3 @@
+
+    @skipIfiOSSimulator
+    @skipIf(compiler="clang")
----------------
labath wrote:
> Do we really need the ios simulator decorator here?
Is this naturally skipped if all OSs are skipped except for linux?

================
Comment at: packages/Python/lldbsuite/test/functionalities/register/intel_xtended_registers/TestMPXRegisters.py:29
@@ +28,3 @@
+    @skipIf(compiler="clang")
+    @expectedFailureAll(oslist=["linux"], compiler="gcc", compiler_version=["<", "5"])
+    @skipIf(archs=no_match(['amd64', 'i386', 'x86_64']))
----------------
labath wrote:
> I presume this is XFAIL because the compiler does not have the required features. If that is true then a "skip" result would be more appropriate.
True, here a skip would be better.

================
Comment at: packages/Python/lldbsuite/test/functionalities/register/intel_xtended_registers/TestMPXRegisters.py:30
@@ +29,3 @@
+    @expectedFailureAll(oslist=["linux"], compiler="gcc", compiler_version=["<", "5"])
+    @skipIf(archs=no_match(['amd64', 'i386', 'x86_64']))
+    def test_mpx_registers_with_example_code(self):
----------------
labath wrote:
> It shouldn't be necessary to specify `amd64` here. I know some old code does that, but now we have code in `lldbtest.py` which automatically remaps it to `x86_64`.
ok, I'll remove it then!

================
Comment at: packages/Python/lldbsuite/test/functionalities/register/intel_xtended_registers/TestMPXRegisters.py:43
@@ +42,3 @@
+
+        self.runCmd('settings set target.inline-breakpoint-strategy always')
+        self.addTearDownHook(
----------------
labath wrote:
> Why is this necessary? (Also it looks like your cleanup function is the same as the setup)
Sorry this is something I should have cleaned up.

================
Comment at: packages/Python/lldbsuite/test/functionalities/register/intel_xtended_registers/TestMPXRegisters.py:50
@@ +49,3 @@
+
+        self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs = ["stop reason = breakpoint 1."])
----------------
labath wrote:
> So, this test will fail if run on hardware which does not have the registers you are testing now (as far as I can tell, that's pretty much all of it). We should detect that situation (the inferior already has code for that, apparently), and skip the test. Something like:
> ```
> if inferior_exited_with_minus_1:
>   self.skipTest("blah blah")
> ```
> 
> 
Good point! I will add this


Repository:
  rL LLVM

https://reviews.llvm.org/D24187