Pseudo-code on #[repr(C)] struct layout contains bugs

[HeroicKatora]

The section on #[repr(C)] type layout contains a (to the best of my knowledge) correct prose description of the alignment algorithm for C structs. However, the pseudo-code that enriches the section does not implement what is indicated in comments and the preceding section. In particular:

struct.alignment = struct.fields().map(|field| field.alignment).max();

let current_offset = 0;

for field in struct.fields_in_declaration_order() {
    // Increase the current offset so that it's a multiple of the alignment
    // of this field. For the first field, this will always be zero.
    // The skipped bytes are called padding bytes.
    // 
    // [SIC!] This line does not achieve at all what the comment specifies.
    // Consider `current_offset = 1`, `field.alignment = 2` for example.
    current_offset += field.alignment % current_offset;

    struct[field].offset = current_offset;

    current_offset += field.size;
}

// [SIC!] Neither does this. This doubles the remainder modulo the 
// structure alignment, it doesn't align it.
struct.size = current_offset + current_offset % struct.alignment;

A correct and reasonably succinct version to compute the offset to some alignment (which works due to the modulus being a power-of-two) is:

// (align - (offset mod align)) mod align
// = (align - offset) mod align // actual integer, not computer word 
// = (2^N - offset) mod align // PoT assumption, align < 2^N
offset.wrapping_neg() % align // 2-complement arithmetic

Fix

A fixed version would be:

struct.alignment = struct.fields().map(|field| field.alignment).max();

let current_offset = 0;

for field in struct.fields_in_declaration_order() {
    // Increase the current offset so that it's a multiple of the alignment
    // of this field. For the first field, this will always be zero.
    // The skipped bytes are called padding bytes.
    current_offset += current_offset.wrapping_neg() % field.alignment;

    struct[field].offset = current_offset;

    current_offset += field.size;
}

// Increase offset to align to struct alignment.
struct.size = current_offset + current_offset.wrapping_neg() % struct.alignment;

---

If stabilized, the code could also utilized the safe abstractions provided here: rust-lang/rust#55724

[ehuss]

Thanks for the report! There's already a PR to fix this at #559. I'll leave a note over there.

[HeroicKatora]

Whoops, I searched bug reports but not PRs directly 😄

[rkanati]

Note: #559 is dead in favour of #766.

[ehuss]

I believe this is now fixed via #766.