Bug 367543 - bt/btc/btr/bts x86/x86_64 instructions are poorly-handled wrt flags
Summary: bt/btc/btr/bts x86/x86_64 instructions are poorly-handled wrt flags
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: vex (show other bugs)
Version: 3.10.0
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-19 03:00 UTC by shortcake
Modified: 2017-05-13 16:55 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description shortcake 2016-08-19 03:00:01 UTC
According to the code, O,S,Z,A,P are being forced to 0, under the rationale that they are "undefined", with no (apparent) user-visible warnings emitted regarding
the effects this has on future instructions with dependencies on these flags, which is less than ideal.

Additionally, the Z flag should be unmodified, not undefined, per real-world CPU behavior and this excerpt(for BT) from Intel's June 2016 architecture manual:
"The CF flag contains the value of the selected bit. The ZF flag is unaffected. The OF, SF, AF, and PF flags are undefined."

Quick and dirty test case derived from a larger program:

int main(int argc, char* argv[])
{
 unsigned a = 0;
 unsigned b = 1;

 asm volatile(
        "testl $15, %%eax\n\t"
        "bt $15, %%ebx\n\t"
        "cmovbe %%ebx, %%eax\n\t"
        : "=a"(a)
        : "a"(a), "b"(b)
        : "cc" );

 __builtin_printf("%u\n", a); 
 if(a != 1)
  __builtin_abort();

 return 0;
}


Reproducible: Always
Comment 1 Julian Seward 2016-09-15 07:12:20 UTC
The Intel documentation has changed, it seems.  The Nov 2007 docs say
(about BT)

  The CF flag contains the value of the selected bit.
  The OF, SF, ZF, AF, and PF flags are undefined.

The Jan 2015 docs say:

  The CF flag contains the value of the selected bit.
  The ZF flag is unaffected. The OF, SF, AF, and PF flags are undefined.

VEX just implements the 2007 version.  I had no idea the Z definition had
changed, until this bug report.

Fixing the Z behaviour sounds reasonable, but regarding O,S,A,P, well, the
docs still list them as undefined.  I am reluctant to fix them to any specific
CPU behaviour.  Also, how would we even know whether a specific CPU 
gives consistent values for those four flags?
Comment 2 Julian Seward 2017-05-13 16:55:31 UTC
Fixed, vex r3367, valgrind r16370.  It now behaves like Skylake:
C is the result, Z is unchanged, and also O,S,A and P are unchanged.

Given that the Intel docs still say that O,S,A and P are undefined,
it seems to me like a (very) bad idea to write software that
assumes anything about their behaviour.