Rand below should take a NonZero parameter

[domenukk]

No description provided.

[domenukk]

It looks more ugly but it makes API users more correct

[tokatoka]

this will have considerably bad impact on performance

[tokatoka]

mutators code is a very hot path, it's better not additional overheads..
also random_corpus_id affects splicing

[domenukk]

this will have considerably bad impact on performance

No it's a zero cost abstraction and the compiler can even use the extra bit for other stuff

[domenukk]

mutators code is a very hot path, it's better not additional overheads.. also random_corpus_id affects splicing

The behavior doesn't change. We can also unsafe unwrap if we feel confident about random_corpus_id...

[tokatoka]

The behavior doesn't change.

The behavor doesn't change, I know. But the assembly code changes.

I'm talking about the code that has expect() or Some(). It will add additional checks to check it is NULL or not, and that overhead is killing mutators.

[tokatoka]

I'm saying this because something similar happened before.
#1138

semantically same code, but the more "Rusty"-code had caused significant slowdown because it added useless instructions wasting CPU cycles.

[tokatoka]

also why can't below() take zero to begin with?
I don't see the problem with the current state

[domenukk]

Right now below doesn't take 0, but it's implicit rather than explicit (with an debug_assert!).
What should below of 0 even be? MAX_INT?

[domenukk]

FWIW the current behavior caused troubles in TNO-S3/WuppieFuzz#5 (comment)

[domenukk]

Other option would be to add an additional below_including function that doesn't take NonZero

[tokatoka]

Other option would be to add an additional below_including function that doesn't take NonZero

This is better. and all the hotpaths in libafl should use that instead.

[domenukk]

Why do you assume NonZero is slower? It can likely be optimized better than the normal version

[tokatoka]

It can likely be optimized better than the normal version

I don't trust the optimization by Rust compiler. As I said, from what I disassembled last time in #1138, it doesn't optimize anything, but just puts useless bound checks just to slow down the stuff.
(Last time, the problem was using std::vec::splice(), it is performing far worse than doing unsafe copy-ing of the buffer. because it adds addtitional bounds checks.)

but if you make this compiles we can always count the instructions or the time spent during the mutations to see how they compare

[domenukk]

It does optimize this pretty well.

Another alternative is to specify that below of 0 is 0.
But that may lead to weird bugs for users (like panics when you use it for items.len() and then access items)

[tokatoka]

It does optimize this pretty well.

in your example yes. but if you put if Some(x) == .. or expect() then there's no way NonZero is better

[domenukk]

It does optimize this pretty well.

in your example yes. but if you put if Some(x) == .. or expect() then there's no way NonZero is better

Most cases that have if Some(x) == would otherwise need a length check or 0 check just above, so it really shouldn't matter

[tokatoka]

I'll show you an example.
#2519 (comment)
This example doesn't have the overhead only because the function square() is actually taking the NonZeroUsize as the argument.
Also it is not similar to the use case of the changes in this PR.

In libafl, you are creating NonZero from usize, be it state.max_size() or input.size() or whatever, it ususally is usize
such as this line https://github.com/AFLplusplus/LibAFL/pull/2519/files#diff-955b4b9ddc30b6a7dcaaa86168ef04730876cbe9714fa570edc3118c740076e1R48 of this PR.

What happens if you create nonzero from usize is
With Nonzero:
https://godbolt.org/z/MTfGE1xsK

Without Nonzero:
https://godbolt.org/z/v59TWvMq3

        cmp     qword ptr [rsp + 272], 0
        sete    al
        test    al, 1
        jne     .LBB4_1
        jmp     .LBB4_2
.LBB4_1:
        lea     rdi, [rip + .L__unnamed_1]
        mov     rax, qword ptr [rip + core::option::unwrap_failed::hba6b08832f9ce30b@GOTPCREL]
        call    rax
.LBB4_2:
        mov     rax, qword ptr [rsp + 272]
        mov     qword ptr [rsp + 16], rax
        lea     rax, [rsp + 16]

THe difference is here, like this, It adds these branches. and I'm saying that even these several lines of additional lines is not good for the mutator.
Of course we should change if it is wrong, but we can simply avoid this overhead by naming below() to below_including().
so we should just keep the current behavior for the lib and swap it with below_including for avoiding users confusion

[domenukk]

You need to optimize in godbolt else it's not useful.
In this case you have the +1 so the compiler can proove unwrap cannot fail. No overhead in this case.
Otherwise it could be a bug, and the unwrap would be the correct way.
Worst case we can always unwrap_unsafe if we want to make it go brrrr

[tokatoka]

NonZero with opt-level=3:
https://godbolt.org/z/axf9nvx4r

without NonZero with opt-level=3:
https://godbolt.org/z/fTeWzsfbz

Still the one without nonzero is better (in terms of the instruction count inside function w)
I mean you can't prove usize to NonZero in general..

[domenukk]

We can always go back to the unsafe variant if we think we don't need the checks https://godbolt.org/z/rY5ebE9ve

[rmalmain]

Other option would be to add an additional below_including function that doesn't take NonZero

something like this? https://github.com/AFLplusplus/LibAFL/pull/2496/files#diff-8b8392ab89ae8517f8910eb579121b57031ada7370c32962b75d0cb62ab841a7R133

[tokatoka]

but your point is that "NonZero version could be optimized" which is not the case. (unless unwrap()_unsafe() is used)
i mean i still don't understand why nonzero is needed in most cases, and sure, we can change below() to take NonZero, but most of the code inside in LibAFL should use below_inclusive().

[rmalmain]

otherwise, why not stick to debug_assert!?
it avoids the overhead in notation and potentially in terms of computation, no?

[domenukk]

otherwise, why not stick to debug_assert!? it avoids the overhead in notation and potentially in terms of computation, no?

That's what we have now and it breaks at runtime instead of compile time. It's objectively worse.

Either we should explicitly allow 0 or explicitly forbid it. NonZero is the compile time way to explicitly forbid it.
If we allow it we should remove the asserts and manually check all users for bugs down the road

[rmalmain]

the other solution could be to have a wrapping type for NonZero which would hide the verbosity?
Maybe you could make this new type implement Deref and internally either unwrap or unwrap_unchecked depending on the build kind?

[tokatoka]

Either we should explicitly allow 0 or explicitly forbid it. NonZero is the compile time way to explicitly forbid it.
If we allow it we should remove the asserts and manually check all users for bugs down the road

So I agree with this

Other option would be to add an additional below_including function that doesn't take NonZero

Expose below() as public function for user to use. for internal ones use below_inclusive()

[domenukk]

We can just NonZero and unwrap_unchecked internally, it's the same in godbolt

[domenukk]

There are not too many constants around that need to be NonZero-ed.
I've used unsafe unwrap_unchecked() wherever possible, so there is no performance overhead in mutators.

[tokatoka]

i need to add debug log to check it like last time

[domenukk]

I'm actually not 100% sure if max_stack_pow of 0 could sometimes make sense(?)
I assumed that not.

[tokatoka]

i'll measure on weekend

[domenukk]

@tokatoka any news? I'd just merge this tbh, there shouldn't be any overhead

[domenukk]

Didn't we even have CI that tests the performance diff?

[tokatoka]

No, don't yet

[domenukk]

Why?

[domenukk]

It's the right thing to do ¯\_(ツ)_/¯

[domenukk]

Poke @tokatoka