fix neg's sign and overflow flags

[LinusU]

From Claude Fable 5.1 (high):

Spotted while auditing which ops set which flags for #19: neg never set SF. Writing a test for that showed OF was wrong too, taken from the unsigned overflow of 0 - x, which is true for every nonzero operand. It should only be set for the most negative value, whose negation is itself.

One commit per flag, plus a test asserting the full flag set for zero, a positive, a negative and 0x80.

[LinusU]

GPT 6 Pro reviewed the changes, and in that process wrote a C + assembly program and ran it 😁

Program

bash -lc cat > /mnt/data/neg-review/neg_native.S <<'EOF'
/* x86-64 System V ABI: (value, initial_rflags, output_rflags_ptr). */
.text
.macro NEG_FN name, operand
.globl \name
.type \name, @function
\name:
    movq %rdi, %rax
    pushq %rsi
    popfq
    neg \operand
    pushfq
    popq %rcx
    cld                         /* Restore the ABI's required DF=0. */
    movq %rcx, (%rdx)
    ret
.size \name, .-\name
.endm
NEG_FN native_neg8, %al
NEG_FN native_neg16, %ax
NEG_FN native_neg32, %eax
NEG_FN native_neg64, %rax
.section .note.GNU-stack,"",@progbits
EOF
cat > /mnt/data/neg-review/neg_check.c <<'EOF'
/*
 * Independent review harness for LinusU/theseus lu-neg-sf.
 * This tests a C transcription of the patched Rust helper against native NEG;
 * it does NOT compile or execute the Rust project.
 *
 * Run on x86-64 Linux:
 *   gcc -O2 -std=c11 -Wall -Wextra -Werror neg_check.c neg_native.S -o neg_check
 *   ./neg_check
 */
#include <inttypes.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>

#define CF UINT64_C(0x001)
#define PF UINT64_C(0x004)
#define AF UINT64_C(0x010)
#define ZF UINT64_C(0x040)
#define SF UINT64_C(0x080)
#define DF UINT64_C(0x400)
#define OF UINT64_C(0x800)
#define PATCH_FLAGS (CF | PF | ZF | SF | OF)
#define STATUS_FLAGS (PATCH_FLAGS | AF)

extern uint64_t native_neg8(uint64_t, uint64_t, uint64_t *);
extern uint64_t native_neg16(uint64_t, uint64_t, uint64_t *);
extern uint64_t native_neg32(uint64_t, uint64_t, uint64_t *);
extern uint64_t native_neg64(uint64_t, uint64_t, uint64_t *);
typedef uint64_t (*native_fn)(uint64_t, uint64_t, uint64_t *);

static uint64_t checks, legacy_mismatches, af_mismatches;

static uint64_t width_mask(unsigned bits) {
    return bits == 64 ? UINT64_MAX : (UINT64_C(1) << bits) - 1;
}

/* Transcribes the new helper's five named flag assignments. */
static uint64_t patched_flags(uint64_t x, uint64_t result,
                              unsigned bits, uint64_t initial) {
    uint64_t f = initial & ~PATCH_FLAGS;
    if (result == 0) f |= ZF;
    if ((result >> (bits - 1)) == 1) f |= SF;
    if (result != 0) f |= CF;
    if ((x >> (bits - 1)) == 1 && (result >> (bits - 1)) == 1) f |= OF;
    if (__builtin_popcount((unsigned)(result & 255)) % 2 == 0) f |= PF;
    return f;
}

/* Independent reference, including AF, from 0 - x semantics. */
static uint64_t reference_flags(uint64_t x, uint64_t result,
                                unsigned bits, uint64_t initial) {
    uint64_t f = initial & ~STATUS_FLAGS;
    const uint64_t sign = UINT64_C(1) << (bits - 1);
    if (x != 0) f |= CF;
    if (x == sign) f |= OF;
    if (result == 0) f |= ZF;
    if (result & sign) f |= SF;
    unsigned parity = 0;
    for (unsigned b = 0; b != 8; ++b) parity ^= (unsigned)((result >> b) & 1);
    if (parity == 0) f |= PF;
    if (x & 15) f |= AF;
    return f;
}

static void check(native_fn fn, unsigned bits, uint64_t x, uint64_t seed) {
    const uint64_t mask = width_mask(bits);
    x &= mask;
    const uint64_t expected_result = (UINT64_C(0) - x) & mask;
    const uint64_t initial = UINT64_C(0x202) | seed;
    uint64_t hardware_flags;
    const uint64_t hardware_result = fn(x, initial, &hardware_flags) & mask;
    const uint64_t patched = patched_flags(x, expected_result, bits, initial);
    const uint64_t reference = reference_flags(x, expected_result, bits, initial);
    if (hardware_result != expected_result ||
        ((patched ^ hardware_flags) & (PATCH_FLAGS | DF)) != 0 ||
        ((reference ^ hardware_flags) & (STATUS_FLAGS | DF)) != 0) {
        fprintf(stderr,
                "FAIL bits=%u x=%016" PRIx64 " seed=%04" PRIx64
                " result=%016" PRIx64 " expected=%016" PRIx64
                " hw_flags=%04" PRIx64 " patched=%04" PRIx64
                " reference=%04" PRIx64 "\n",
                bits, x, seed, hardware_result, expected_result,
                hardware_flags, patched, reference);
        exit(EXIT_FAILURE);
    }
    /* Reconstruct old SF/OF behavior to make sure the suite detects the bugs. */
    uint64_t old = (patched & ~(SF | OF)) | (initial & SF);
    if (x != 0) old |= OF;   /* Unsigned 0-x underflows for every nonzero x. */
    legacy_mismatches += ((old ^ hardware_flags) & PATCH_FLAGS) != 0;
    af_mismatches += ((patched ^ hardware_flags) & AF) != 0;
    ++checks;
}

static uint64_t all_flag_seed(unsigned index) {
    static const uint64_t flags[] = {CF, PF, AF, ZF, SF, OF, DF};
    uint64_t seed = 0;
    for (unsigned b = 0; b != 7; ++b)
        if (index & (1u << b)) seed |= flags[b];
    return seed;
}

static uint64_t random64(void) {
    static uint64_t state = UINT64_C(0x2039fefef01103da);
    state ^= state >> 12;
    state ^= state << 25;
    state ^= state >> 27;
    return state * UINT64_C(2685821657736338717);
}

static void exhaustive(native_fn fn, unsigned bits) {
    uint64_t before = checks;
    for (uint64_t x = 0; x <= width_mask(bits); ++x)
        for (unsigned seed = 0; seed != 128; ++seed)
            check(fn, bits, x, all_flag_seed(seed));
    printf("%u-bit exhaustive: %" PRIu64 " values x 128 flag seeds = %" PRIu64
           " checks: PASS\n", bits, width_mask(bits) + 1, checks - before);
}

static void sampled(native_fn fn, unsigned bits) {
    uint64_t before = checks;
    const uint64_t sign = UINT64_C(1) << (bits - 1);
    const uint64_t mask = width_mask(bits);
    uint64_t edges[] = {0, 1, 2, 15, 16, 17, 127, 128, 129, 255, 256, 257,
                        sign - 1, sign, sign + 1, mask - 1, mask,
                        UINT64_C(0xaaaaaaaaaaaaaaaa) & mask,
                        UINT64_C(0x5555555555555555) & mask,
                        (-UINT64_C(0x101)) & mask};
    for (size_t i = 0; i != sizeof(edges) / sizeof(edges[0]); ++i)
        for (unsigned seed = 0; seed != 128; ++seed)
            check(fn, bits, edges[i], all_flag_seed(seed));
    printf("%u-bit boundary/pattern cases: %" PRIu64 " checks: PASS\n",
           bits, checks - before);
    before = checks;
    const uint64_t seeds[] = {0, STATUS_FLAGS, DF, STATUS_FLAGS | DF};
    for (unsigned i = 0; i != 1000000; ++i) {
        uint64_t x = random64() & mask;
        for (unsigned s = 0; s != 4; ++s) check(fn, bits, x, seeds[s]);
    }
    printf("%u-bit deterministic pseudorandom: 1,000,000 values x 4 seeds = %"
           PRIu64 " checks: PASS\n", bits, checks - before);
}

int main(void) {
    exhaustive(native_neg8, 8);
    exhaustive(native_neg16, 16);
    sampled(native_neg32, 32);
    sampled(native_neg64, 64);
    printf("TOTAL: %" PRIu64 " checks; zero patched CF/PF/ZF/SF/OF, result, "
           "or DF-preservation mismatches.\n", checks);
    printf("Old SF/OF implementation: %" PRIu64 " failing test cases.\n",
           legacy_mismatches);
    printf("Pre-existing untouched AF: %" PRIu64 " mismatches (excluded "
           "from patch pass criterion).\n", af_mismatches);
    printf("Independent AF formula (x & 0xf) != 0: PASS against hardware.\n");
    return EXIT_SUCCESS;
}
EOF
gcc -O2 -std=c11 -Wall -Wextra -Werror /mnt/data/neg-review/neg_check.c /mnt/data/neg-review/neg_native.S -o /mnt/data/neg-review/neg_check
/mnt/data/neg-review/neg_check | tee /mnt/data/neg-review/neg_check_results.txt

`results.txt`

Theseus NEG review
Base: 2294324220d38e8a4f94005bbf315cb79351c1f6
Head: b3c4f78c16b0d70a2a8743434a6c03e60c3e4332
Host: x86_64; /proc/cpuinfo reports INTEL(R) XEON(R) PLATINUM 8573C
Compiler: GCC 14.2.0

Scope: C transcription of the Rust NEG helper; not a Theseus build.
Oracle: native x86 NEG and immediately captured RFLAGS.
8-bit and 16-bit: exhaustive inputs, each under all 64 initial arithmetic-flag combinations.
32-bit and 64-bit: 14 boundary inputs under all 64 combinations, plus
100,000 deterministic pseudorandom samples under all-clear/all-set arithmetic flags.
DF preservation is checked as well. AF omission is counted separately.

Compiled with -O2 -mno-red-zone -Wall -Wextra -Werror.
Repeated with -O0 -fsanitize=undefined; identical output, no sanitizer errors.
The counts below are PER RUN, not summed across the two runs.

 8-bit: cases=16384; patched_result_and_5_flags_plus_DF_errors=0; old_OF_errors=16256; old_SF_errors=8192; preexisting_AF_omission_errors=8192; proposed_AF_formula_errors=0
16-bit: cases=4194304; patched_result_and_5_flags_plus_DF_errors=0; old_OF_errors=4194176; old_SF_errors=2097152; preexisting_AF_omission_errors=2097152; proposed_AF_formula_errors=0
32-bit: cases=200896; patched_result_and_5_flags_plus_DF_errors=0; old_OF_errors=200768; old_SF_errors=100448; preexisting_AF_omission_errors=100448; proposed_AF_formula_errors=0
64-bit: cases=200896; patched_result_and_5_flags_plus_DF_errors=0; old_OF_errors=200768; old_SF_errors=100448; preexisting_AF_omission_errors=100448; proposed_AF_formula_errors=0
TOTAL: 4612480 native NEG executions

It concluded:

Yes. Both changes are correct, and I would approve this as a targeted fix for NEG’s sign and overflow flags. I reviewed the comparison from 2294324 to b3c4f78, checked Intel’s instruction and flag definitions, and independently tested the formulas against native x86 execution.

It also flagged another issue that should be fixed as well:

Intel specifies that NEG updates AF too. It is not undefined or preserved. AF records the carry or borrow out of bit 3; for 0 - x, that gives:

let af = (x.low_byte() & 0x0f) != 0;

This follows because subtracting a nonzero low nibble from zero requires a borrow. The native tests confirmed that formula.

The current Flags definition has no named AF constant, and neither the old nor new neg() updates bit 4. Therefore, the test strings are correct for the flags being checked, but they are not a complete description of every arithmetic flag on hardware: for example, negating 0x01 or 0xFF also sets AF.

I'll send a follow up PR for this 🚀

edit: just realized that the AF flag is missing from the entire project, so not a small one liner 😅