alloc debug overflow on size 0

[m4b]

if the size is 0, this will abort on a sub overflow in debug mode.

    fn alloc(
        &mut self,
        device: &B::Device,
        size: u64,
        align: u64,
    ) -> Result<(DynamicBlock<B>, u64), gfx_hal::device::AllocationError> {
        debug_assert!(size <= self.max_allocation());
        debug_assert!(align.is_power_of_two());
        let aligned_size = ((size - 1) | (align - 1) | (self.block_size_granularity - 1)) + 1;

[zakarumych]

Size and align 0 are not supported. Should add this to asserts as nd doc.

[m4b]

please don't assert if size is 0. just return an allocation error if possible, or at least do a checked sub and unwrap so it doesn't do weird stuff in release mode; but ideally just return an error and let the client decide if they want to crash the process?

[zakarumych]

I don't want to add another error variant. Especially since we use error type from gfx-hal. Creating buffer of size 0 violates Vulkan API usage already and it typically happens before memory allocation so we already in dangerous waters where anything can happen.

Doing checking sub and unwrapping is much worse than meaningful assert IMHO. You can't allocate 0 bytes not because of this expression (it can be modified to permit 0) so unwrapping there will only confuse the user.

[m4b]

ok, agreed assert better than debug overflow unwrap; even better it's caught even farther upstream, but yea.

Is creating a buffer of size 0 in vulkan really a validation error? I would assume it wouldn't care; it should be able to hand out an infinite amount of 0 size buffers ; )

[zakarumych]

Yes. Creating buffers of size 0 is invalid usage.

[zakarumych]

I think we can just add asserts in debug mode.

[paulocsanz]

@omni-viral

Shouldn't the assert also be there in release? Since in release it will wrap and a bunch of unexpected things will happen?

[m4b]

If it’s invalid usage for size 0 simple panic with error message is best imho, and then documenting this as a panics condition :)

Asserts are for pre and post condition enforcement and author programmer errors

[zakarumych]

@m4b std uses asserts for invalid inputs like out of bounds for some Vec methods etc. So I'd say assert is idiomatic way in Rust.

[m4b]

Out of bounds panics with bounds len violation message, last I checked. I’m sure there are asserts in vec but I can’t remember last time an assert violation bubbled up to the user but I could be mistaken.

[m4b]

In any event I mostly care about fixing the debug overflow which wasn’t super helpful wrt tracking down cause, and I don’t really care if it’s a panic or assert, just a better error message. This is effectively a one line fix we’re talking about so maybe we should just add it :)