gcc miscompilation in Stockfish 1.6

[zamar]

Note that here compiler is free to choose between signed and unsigned type:

Code: Select all

enum Score &#123; ENSURE_32_BIT_SIZE = 1 << 31 &#125;;

while here compiler is forced to use signed int:

Code: Select all

enum Score &#123; ENSURE_32_BIT_SIZE_MAX = 1 << 31,  ENSURE_32_BIT_SIZE_MIN = -&#40;1<<31&#41;&#125;;

Or perhaps more easily:

Code: Select all

enum Score &#123; ENSURE_32_BIT_SIZE_MAX = 1 << 31, ENSURE_NEGATIVE=-1 &#125;;

[mcostalba]

Note that if enum is (a signed) int, and the Scores can be negative (which I think they can for you), then the result of that >> 16 is entirely implementation defined.

Sorry, I don't understand this. Why should be an error a right shift of a negative number ? I would think the right shift of an int is sign extended.

Care to explain ? Thanks.

BTW according to your reasoning, if I have understood correctly, should be wrong also this one:

Code: Select all

inline Value mg_value(Score s) { return Value((int(s) + 32768) >> 16); }

[Jan Brouwer]

How is Value exactly defined?
I didn't find in this thread (maybe I missed it).

Jan

[mcostalba]

How is Value exactly defined?
I didn't find in this thread (maybe I missed it).

Jan

It is defined just few lines above Score in value.h file

Code: Select all

enum Value {
  VALUE_DRAW = 0,
  VALUE_KNOWN_WIN = 15000,
  VALUE_MATE = 30000,
  VALUE_INFINITE = 30001,
  VALUE_NONE = 30002
};

[Gian-Carlo Pascutto]

Sorry, I don't understand this. Why should be an error a right shift of a negative number ?

It's not an error, it's implementation defined. Meaning, if you switch compilers or compiler versions, or even optimization settings, the result is allowed to change.

If you care for the exact outcome (which I would presume), then the code is wrong.

I would think the right shift of an int is sign extended.

There is nothing in the C standard that guarantees that. See section 6.5.7, point 5:

http://www.open-std.org/JTC1/SC22/WG14/ ... /n1256.pdf

[mcostalba]

Sorry, I don't understand this. Why should be an error a right shift of a negative number ?

It's not an error, it's implementation defined. Meaning, if you switch compilers or compiler versions, or even optimization settings, the result is allowed to change.

If you care for the exact outcome (which I would presume), then the code is wrong.

I would think the right shift of an int is sign extended.

There is nothing in the C standard that guarantees that. See section 6.5.7, point 5:

http://www.open-std.org/JTC1/SC22/WG14/ ... /n1256.pdf

Thanks,

so I presume also this one should be changed

Code: Select all

inline Value mg_value(Score s) { return Value((int(s) + 32768) >> 16); } 

Have you a suggestion on how to extract the _signed_ 16 bits of the lower and upper part of a 32 bit field (integer) ?

[Sven]

Note that if enum is (a signed) int, and the Scores can be negative (which I think they can for you), then the result of that >> 16 is entirely implementation defined.

Sorry, I don't understand this. Why should be an error a right shift of a negative number ? I would think the right shift of an int is sign extended.

Care to explain ? Thanks.

BTW according to your reasoning, if I have understood correctly, should be wrong also this one:

Code: Select all

inline Value mg_value(Score s) { return Value((int(s) + 32768) >> 16); }

From C++ standard which essentially contains the same text as the C standard pointed to by Gian-Carlo regarding shift operators:

5.8 Shift operators
[...]
3 The value of E1 >>E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a nonnegative value, the value of the result is the integral part of the quotient of E1 divided by the quantity 2 raised to the power E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined.

Sven

[mcostalba]

There is nothing in the C standard that guarantees that. See section 6.5.7, point 5:

http://www.open-std.org/JTC1/SC22/WG14/ ... /n1256.pdf

Ok, so what about this one ?

Code: Select all

inline Value eg_value(Score s) { return Value((int)(unsigned(s) & 0x7fffu) - (int)(unsigned(s) & 0x8000u)); }

I have verified it works with all the compilers and does not rely on shifts or cast to shorts. I have found it googling around and it seems safe to me.

[Gerd Isenberg]

so I presume also this one should be changed

Code: Select all

inline Value mg_value(Score s) { return Value((int(s) + 32768) >> 16); } 

Have you a suggestion on how to extract the _signed_ 16 bits of the lower and upper part of a 32 bit field (integer) ?

You may use div instead of shift right:

Code: Select all

inline Value mg_value(Score s) {return Value(int(s & ~0xffff) / 0x10000);} 
inline Value eg_value(Score s) {return Value(int(short(s &  0xffff)));} 

Likely x86 compiler will emit the intended x86 sar instruction instead of div, as mentioned in the Software Optimization Guide for AMD Family 10h Processors (pdf) pg 122:

Code: Select all

Signed Division by 2^n
; In: EAX = dividend
; Out: EAX = quotient
cdq ; Sign extend into EDX.
and edx, (2^n - 1) ; Mask correction (use divisor - 1)
add eax, edx ; Apply correction if necessary.
sar eax, (n) ; Perform right shift by log2(divisor).

[Zach Wegner]

Note that here compiler is free to choose between signed and unsigned type:

Code: Select all

enum Score &#123; ENSURE_32_BIT_SIZE = 1 << 31 &#125;;

while here compiler is forced to use signed int:

Code: Select all

enum Score &#123; ENSURE_32_BIT_SIZE_MAX = 1 << 31,  ENSURE_32_BIT_SIZE_MIN = -&#40;1<<31&#41;&#125;;

No, for the last one it would have to use something more than 32 bit. In 32 bits, (1<<31) and -(1<<31) are the same number. And in the first example, it would have to use unsigned ints, bit 31 is the sign bit so (1<<31) is negative in signed arithmetic. Your examples would work if you change the 31s to 30s, though.