Fix incorrect pseudocode for #[repr(C)] struct alignment

[kinseytamsin]

Alternative to #559 incorporating some suggested changes to that PR.

@RalfJung I felt like your pseudocode approached actual code too much, and sorta requires understanding what's going on with Layout in order to follow, so I just took the route of making the padding calculation into an implicit function per other suggestions.

[rkanati]

Neat! I'll nix mine.

[kinseytamsin]

@rkanati I assumed that the fact that the original PR stalled meant that you weren't likely to show up, I kinda wanted you to be able to fix yours and get the credit 😭 But I also wanted it done, so I guess I haven't really done anything wrong XD

[rkanati]

No problem! It's just a doc fix, after all.

[RalfJung]

I agree now that it's better not to use Layout, to keep this code self-contained.

However, could you please define padding_needed_for? As we have seen, it is somewhat subtle to get right.

[RalfJung]

@rkanati you didn't touch this one in your PR... do you remember why? Was that an oversight, or was the original code actually correct for some reason?

[kinseytamsin]

As far as I can tell, that was just an oversight.

[RalfJung]

I think so, too. It would just have been good to get explicit confirmation.

[kinseytamsin]

@RalfJung Yeah, I started including a definition of the function but wasn't sure whether that would be necessary. Luckily now that I know about Layout (and even which method to look at), I can just copy that code :)

[kinseytamsin]

Okay so, for reference, here's the code for Layout::padding_needed_for.

pubfnpadding_needed_for(&self,align:usize) -> usize{let len = self.size();// Rounded up value is:// len_rounded_up = (len + align - 1) & !(align - 1);// and then we return the padding difference: `len_rounded_up - len`.//// We use modular arithmetic throughout://// 1. align is guaranteed to be > 0, so align - 1 is always// valid.//// 2. `len + align - 1` can overflow by at most `align - 1`,// so the &-mask with `!(align - 1)` will ensure that in the// case of overflow, `len_rounded_up` will itself be 0.// Thus the returned padding, when added to `len`, yields 0,// which trivially satisfies the alignment `align`.//// (Of course, attempts to allocate blocks of memory whose// size and padding overflow in the above manner should cause// the allocator to yield an error anyway.)let len_rounded_up = len.wrapping_add(align).wrapping_sub(1)& !align.wrapping_sub(1);
len_rounded_up.wrapping_sub(len)}

I'm feeling like this might be a bit overkill to directly adapt into what's supposed to be pseudocode. @RalfJung do you think I should use this directly for the definition of padding_needed_for in the pseudocode or should I write a definition according to a naive definition of the algorithm (i.e., without the measures to account for extreme values/overflowing)?

I'm actually having a bit of trouble fully understanding what's going on in that code, what with all the layered modular arithmetic and bitwise operations (I'm not really fully acquainted with the identities and such to be able to easily follow what bitwise operations are doing, so after puzzling it out a bit it's turning out to be extremely non-obvious to me how to translate that into a simple presentation of the algorithm suited to pseudocode). Maybe @pnkfelix can help since they wrote that code?

[RalfJung]

I was actually thinking of using the implementation from the other PR:

fn align_to_padding(offset: usize, alignment: usize) -> usize { // or whatever type seems appropriate?
let misalignment = current_offset % alignment;
if misalignment > 0 {
current_offset - misalignment + alignment // round up to next multiple of `alignment`
} else {
current_offset // already a multiple of `alignment`
}
}

Maybe also add a note somewhere that we are ignoring overflow concerns here, and link to Layout for people that actually want to do these computations in real code.

[kinseytamsin]

@RalfJung Yeah, I'll do that. I'm definitely overthinking this, heh.

[kinseytamsin]

@RalfJung I added a function definition to the pseudocode as requested. I agree that the note directing the reader to Layout::padding_needed_for is a good idea, but should it be a note or a warning? I figured I'd ask for a second opinion before I actually add that, to be sure.

[RalfJung]

What do you mean, "warning"? What would it warn about?

[RalfJung]

Oh you mean, the fact that we ignore overflows. Yeah seems best to me to call it a warning, we don't want people to copy-paste this code.

[kinseytamsin]

@RalfJung Any further concerns with this PR? 🙂 (Besides the need to squash these commits before merging.)

[RalfJung]

So the code looks all right to me now, but I'd prefer to get this confirmed by some data layout guru... summoning @gnzlbg@hanna-kruppe@eddyb

[kinseytamsin]

@RalfJung All your review comments should be addressed now 🙂

I just edited the existing commits, since the requested changes were pretty minor.

[RalfJung]

Thanks, I think they are. :)

Now we are just waiting for some extra eyes on this.

[hanna-kruppe]

Looks fine to me too, thanks @kinseytamsin!

[RalfJung]

Looks like we are settled then.