Remove illegal instruction from tests #210

ranweiler · 2017-12-23T02:15:50Z

Closes False positives in tests #205
Reverts the commit that closed More accurate floating-point rounding in X86 FMA extension instructions #168 (see here)
Includes segment registers in saved test state, as suggested here

Tests pass locally for me, but CI fails right now, likely due to #187 being hidden by #205.

The code segment (CS) register cannot be explicitly loaded, unlike the other segment registers. The instruction `mov cs, rax` is thus illegal, and triggered a SIGILL for each test case, causing all tests to spuriously succeed.

The commit that first added this included an illegal instruction, which caused false positives and hid the fact that this is redundant.

This reverts commit 9a9f370. The bug in lifting-bits#205 is what made it seem like the underlying issue here had been addressed.

pgoodman · 2017-12-23T02:47:11Z

I'm seeing lots of failures for fcmov-related instructions on Travis, e.g.

E1223 02:31:53.966231 10449 Run.cpp:566] States did not match for FCMOVNU_1 with ARG1=0x0 and CF=0 PF=0 AF=0 ZF=0 SF=0 DF=0 OF=0
/home/travis/build/trailofbits/remill/tests/X86/Run.cpp:568: Failure
Value of: !"Lifted and native states did not match."
  Actual: false
Expected: true

pgoodman · 2017-12-23T02:48:45Z

I think what is happening is that the DEF_FPU_SEM macro is updating the fpu cs, and that maybe is not quite right anymore?

ranweiler · 2017-12-23T03:06:30Z

@pgoodman I just did one last double-check on my local box right now with fully-rebuilt Docker images, and actually am getting some of the CI errors— I think I had some stale state when locally testing the last commit. I'm going to try it (a) without checking the CS register, (b) without saving any segment registers.

pgoodman · 2017-12-23T03:07:17Z

This could be an artefact of the test runner being a 64-bit program, even when running the 32-bit tests. Specifically, we'll need to figure out if the running fxsave in a 32-bit program saves the cs, where the equivalent fxsave in a 64-bit program doesn't.

pgoodman · 2017-12-23T03:09:25Z

What I'm getting at is that, the fxsave64 format doesn't record the cs, so maybe behind the scenes, 64-bit mode doesn't record the cs either, and so it just zeroes it out when using fxsave.

ranweiler · 2017-12-23T03:14:05Z

@pgoodman that's consistent with what I see locally on a totally fresh image: I'm saving all the segment registers except CS, and all tests pass. I could amend the last commit to save all the seg registers except CS, and we could open a separate issue for the 32/64-bit(?) CS situation.

ranweiler · 2017-12-23T03:37:57Z

Okay, not including the CS register gets rid of almost all of the CI test failures (on that run, anyway). Locally, I observe no test failures if I exclude the CS register.

ranweiler · 2017-12-23T04:03:09Z

I've amended the last commit to exclude CS, which ensures we have only CI failures for now.

pgoodman · 2017-12-23T05:00:23Z

I figure this is the source of the issue:

I think the "right" thing to do is re-add your saving of cs in the PrintSaveState.cpp, and then perhaps in the Run.cpp, what we can do is a cpuid check on whether or not we should bother checking cs in the fxsave, and if not, zero out the lifted version (which does save it).

pgoodman · 2017-12-23T22:19:27Z

tests/X86/Run.cpp

+//    CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1
+//
+// Where "bit 13" is a 0-based index.
+static bool AreFCSAndFDSDeprecated() {


(void) as param list.

pgoodman · 2017-12-23T22:20:12Z

tests/X86/Run.cpp

@@ -458,6 +490,14 @@ static void RunWithFlags(const test::TestInfo *info,
  memset(lifted_state->x87.fxsave.st, 0, kill_size);
  memset(native_state->x87.fxsave.st, 0, kill_size);

+#if 32 == ADDRESS_SIZE_BITS
+  // If FCS and FDS are deprecated, don't compare them.
+  if (AreFCSAndFDSDeprecated()) {


if (32 == ADDRESS_SIZE_BITS && ...), that way we don't get a compiler warning about AreFCSAndFDSDeprecated being unused.

pgoodman · 2017-12-23T22:37:52Z

tests/X86/Run.cpp

@@ -458,6 +490,14 @@ static void RunWithFlags(const test::TestInfo *info,
  memset(lifted_state->x87.fxsave.st, 0, kill_size);
  memset(native_state->x87.fxsave.st, 0, kill_size);

+#if 32 == ADDRESS_SIZE_BITS


I meant to remove the #if and just depend on the actual if :-P

Oh, that makes more sense (-:

The reason I used #if is because the referenced fields don't exist in 64-bit mode, so you get a compiler error.

Oh duh! I didn't make that connection.. lol. So then the additional check in the if is unnecessary.

Apparently not? One moment...

Nope, yes, the #if is necessary, I just assumed you were talking about some other warning I missed. Reverting the redundant expression.

ranweiler added 3 commits December 22, 2017 17:18

Remove illegal instruction from tests (lifting-bits#205)

066ad2a

The code segment (CS) register cannot be explicitly loaded, unlike the other segment registers. The instruction `mov cs, rax` is thus illegal, and triggered a SIGILL for each test case, causing all tests to spuriously succeed.

Remove unneeded DS register preservation around test cases

954a990

The commit that first added this included an illegal instruction, which caused false positives and hid the fact that this is redundant.

Revert "Restore previously failing FMA tests"

7b05caa

This reverts commit 9a9f370. The bug in lifting-bits#205 is what made it seem like the underlying issue here had been addressed.

pgoodman approved these changes Dec 23, 2017

View reviewed changes

ranweiler force-pushed the remove-illegal-test-instr branch from 61c4f3d to ca8b92c Compare December 23, 2017 03:54

ranweiler force-pushed the remove-illegal-test-instr branch 2 times, most recently from 37566e9 to bd3779c Compare December 23, 2017 04:22

Include segment registers in SaveState()

d1e36d0

ranweiler force-pushed the remove-illegal-test-instr branch from bd3779c to 57a1cb2 Compare December 23, 2017 21:43

pgoodman requested changes Dec 23, 2017

View reviewed changes

ranweiler force-pushed the remove-illegal-test-instr branch from 57a1cb2 to 4d1bcd6 Compare December 23, 2017 22:23

pgoodman reviewed Dec 23, 2017

View reviewed changes

If in 32-bit mode, check if we should zero out x87 CS

14b7877

ranweiler force-pushed the remove-illegal-test-instr branch from 4d1bcd6 to 14b7877 Compare December 23, 2017 22:45

pgoodman approved these changes Dec 23, 2017

View reviewed changes

pgoodman merged commit b50d29f into lifting-bits:master Dec 23, 2017

ranweiler deleted the remove-illegal-test-instr branch December 23, 2017 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove illegal instruction from tests #210

Remove illegal instruction from tests #210

ranweiler commented Dec 23, 2017 •

edited

Loading

pgoodman commented Dec 23, 2017

pgoodman commented Dec 23, 2017

ranweiler commented Dec 23, 2017

pgoodman commented Dec 23, 2017

pgoodman commented Dec 23, 2017

ranweiler commented Dec 23, 2017

ranweiler commented Dec 23, 2017

ranweiler commented Dec 23, 2017

pgoodman commented Dec 23, 2017

pgoodman Dec 23, 2017

pgoodman Dec 23, 2017

pgoodman Dec 23, 2017

ranweiler Dec 23, 2017

pgoodman Dec 23, 2017

ranweiler Dec 23, 2017

ranweiler Dec 23, 2017

Remove illegal instruction from tests #210

Remove illegal instruction from tests #210

Conversation

ranweiler commented Dec 23, 2017 • edited Loading

pgoodman commented Dec 23, 2017

pgoodman commented Dec 23, 2017

ranweiler commented Dec 23, 2017

pgoodman commented Dec 23, 2017

pgoodman commented Dec 23, 2017

ranweiler commented Dec 23, 2017

ranweiler commented Dec 23, 2017

ranweiler commented Dec 23, 2017

pgoodman commented Dec 23, 2017

pgoodman Dec 23, 2017

Choose a reason for hiding this comment

pgoodman Dec 23, 2017

Choose a reason for hiding this comment

pgoodman Dec 23, 2017

Choose a reason for hiding this comment

ranweiler Dec 23, 2017

Choose a reason for hiding this comment

pgoodman Dec 23, 2017

Choose a reason for hiding this comment

ranweiler Dec 23, 2017

Choose a reason for hiding this comment

ranweiler Dec 23, 2017

Choose a reason for hiding this comment

ranweiler commented Dec 23, 2017 •

edited

Loading