Avoid using feature flags and env variable to set the same parameter pt.1 emulation_mode

[Marcondiro]

This PR follows discussion https://github.com/AFLplusplus/LibAFL/discussions/2505 TLDR: Using only the feature flag simplifies things a bit and allows the usage of optional dependencies

[Marcondiro]

WIP, for now I've done it just for emulation_mode, any feedback is welcome

[Marcondiro]

Ubuntu-clippy CI fails because it is run with --all-features. Upstream doesn't fail because it silently defaults to usermode, clippy is not run for systemmode

[domenukk]

--all-features should enabled a clippy feature which makes it work magically. Maybe just build usermode always?

[Marcondiro]

@domenukk I'm not sure I fully understood your message :smile: How about avoiding --all-features for libafl_qemu? I don't like this solution very much and maybe in the future the incompatible features thing could be reconsidered

[domenukk]

@domenukk I'm not sure I fully understood your message 😄 How about avoiding --all-features for libafl_qemu? I don't like this solution very much and maybe in the future the incompatible features thing could be reconsidered

running clippy --all-features (IMHO) should just work(tm)

[domenukk]

Like, why make this so much harder than it needs to be?

[Marcondiro]

The thing is that in libafl_qemu some features are incompatible (usermode and systemmode for instance), running with --all-features should fail.

So far the build script decided which feature to actually use and which silently ignore when running with --all-features. In my opinion having --all-features just working but actually not enabling all features is not ideal. Looking at the clippy script it looks like every feature of libafl_qemu is checked with clippy while it is not.

I thought this change would simplify and clean things a bit (no hidden defaults, fallback env vars, just plain feature), but if it looks more convoluted to you I'll just drop this PR no problem :)

[domenukk]

Features should be additive (see: https://doc.rust-lang.org/nightly/cargo/reference/features.html#feature-unification ) so I think we should either

• specify a default for what happens when both features are set or
• take away the usermode feature and make it the default. Note that this is likely not possible for architectures, so...

I'm not sure what other crates do that have mutual exclusive dependencies, like crypto libs with backends etc?

[Marcondiro]

Right below the link you've posted there is also: https://doc.rust-lang.org/nightly/cargo/reference/features.html#mutually-exclusive-features

This should be avoided if possible and yes features should be additive, this is why I was saying

I don't like this solution very much and maybe in the future the incompatible features thing could be reconsidered

About

specify a default for what happens when both features are set

It is more or less what is done now, IMHO this should fail at compile time (also according to the above link)

take away the usermode feature and make it the default. Note that this is likely not possible for architectures, so...

As you say feature are meant to be additive, here again we would also remove/replace stuff

[domenukk]

The link you posted literally states:

Instead of using mutually exclusive features, consider some other options: ...

• When there is a conflict, choose one feature over another. The cfg-if package can help with writing more complex cfg expressions.

In this case it's easy - we default to usermode and x86_64, that's what we've done so far

[domenukk]

BTW I don't care either way, @rmalmain and/or @andreafioraldi should say how they want it

[andreafioraldi]

if there is a conflict i would fail with an error, as we already do for x86+big endian for instance

[domenukk]

Fine by me - but maybe if we still have the clippy feature that makes things "just work" --all will still do the right thing here?

[rmalmain]

we can also adapt clippy flags in the clippy script is the CI variable is on. i don't think it makes sense to change this kind of general behaviour only to have clippy --all working.
i think we shouldn't use env variables to set / unset features in general, it is confusing otherwise. btw @Marcondiro, i think you shouldn't use unreachable! but panic! instead with an error message, it should be more clear to understand the problem.

[domenukk]

What's the status here? Needs a rebase and then needs to get reviewed/merged @rmalmain

[Marcondiro]

@rmalmain should be ready for review

[rmalmain]

it's hard to tell if your pr is gonna break something at the moment, half of the CI is broken