official-stockfish / Stockfish

A free and strong UCI chess engine
https://stockfishchess.org/
GNU General Public License v3.0
11.33k stars 2.25k forks source link

PawnAttacks from rank 1 and 8 #2219

Closed mstembera closed 5 years ago

mstembera commented 5 years ago

On line 94 in bitboard.cpp

PawnAttacks[c][s] |= to;

we initialize the PawnAttacks array. We do this even when the square s is on rank 1 or 8 which seems wrong since pawns can never exist on those ranks. When I add

if (rank_of(s) != RANK_1 && rank_of(s) != RANK_8)

just above to avoid this the resulting binary crashes during bench. This means we are making use of these pawn attacks from rank 1 or 8 somewhere else in the code. Maybe this is somehow ok but it seems suspect. Does anyone know the reason behind this?

mstembera commented 5 years ago

Note: I am scoping the above if properly using { } so that the else for PseudoAttacks is matching the correct if.

if (pt == PAWN)
{
    if (rank_of(s) != RANK_1 && rank_of(s) != RANK_8)
        PawnAttacks[c][s] |= to;
}
else
    PseudoAttacks[pt][s] |= to;
hxim commented 5 years ago

Does anyone know the reason behind this?

I think this line: https://github.com/official-stockfish/Stockfish/blob/ca51d1ee63f376e0eb6efb6f3d5d901e4b2a5bb0/src/pawns.cpp#L105

mstembera commented 5 years ago

@hxim Thanks. I changed it to

if (rank_of(s + Up) != RANK_1 && rank_of(s + Up) != RANK_8)
    leverPush = theirPawns & PawnAttacks[Us][s + Up];
else
    leverPush = 0;

and everything is fine w/ same bench signature so that's not it.
I'm just wondering in general if we have some bug here or not?

mstembera commented 5 years ago

BTW, the reason I'm not easily able to debug it myself is that in debug mode I simply hit the assert

assert(type_of(captured) != KING);

on line 751 in position.cpp by which point it's too late to know what caused it. Someone more familiar w/ this code than me may have a better chance.

ianfab commented 5 years ago

@mstembera Since I encountered this failed assertion dozens of times when impementing chess variants, I am quite sure that it boils down to the below code: https://github.com/official-stockfish/Stockfish/blob/ca51d1ee63f376e0eb6efb6f3d5d901e4b2a5bb0/src/position.cpp#L515-L518

For pawns we use a symmetry here and if we apply your suggested change, the symmetry is broken and breaks attachers_to for pawns on the seventh rank. Since this is, e.g., used for the king in legal, SF will finally play a move that leaves the king in check, so on the next move the king is captured and you get the failed assertion.

mstembera commented 5 years ago

@ianfab Thanks! I believe your explanation is correct. Just for completeness these lines

b1 &= pos.attacks_from<PAWN>(ksq, Them);
b2 &= pos.attacks_from<PAWN>(ksq, Them);

in movegen.cpp are also affected in that the bench changes and, this line in position.cpp Position::set_check_info()

si->checkSquares[PAWN]   = attacks_from_full<PAWN>(ksq, ~sideToMove);

causes a crash when ranks 1 & 8 are omitted. I will leave this open a bit longer in case anyone else has additional insights or comments.

Rocky640 commented 5 years ago

To summarize

1) we need PawnAttacks[WHITE][SQ_G1] to be f2 and h2 to find out if a Kg1 is under check by a pawn.

2) The only case where we want PawnAttacks[c][s] = 0 is a) c==WHITE and s>SQ_H7 b) c==BLACK and s<SQ_A2 to avoid undefined behavior.

But we already do this thanks to line 90 in bitboard.h if (is_ok(to) && distance(s, to) < 3)

So current master is fine,

mstembera commented 5 years ago

Thanks to everyone who commented!