zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.58k stars 6.47k forks source link

Disable CMake warning for enabled asserts #78038

Open benediktibk opened 3 weeks ago

benediktibk commented 3 weeks ago

Whenever CONFIG_ASSERT is enabled in a normal build (=not CONFIG_TEST) a cmake warning is triggered. This warning has already been reduced in its verbosity in #21223.

I would tend to remove it from warning completely (either to INFO or VERBOSE), because I usually keep CONFIG_ASSERT enabled even in production builds. I consider this as a safety measure, as I would rather have the system halt than keep going in an undefined state.

Probably of interest for @ioannisg @nashif @SebastianBoe @joerchan @aescolar

henrikbrixandersen commented 3 weeks ago

I think the warning should stay.

Enabling CONFIG_ASSERT comes with both a footprint increase and a runtime penalty, which should be a bough to warrant a warning to the user.

palchak-google commented 1 week ago

I think the warning should stay.

Enabling CONFIG_ASERT comes with both a footprint increase and a runtime penalty, which should be a bough to warrant a warning to the user.

By that logic we need to add CMake warnings for whenver CONFIG_DEBUG or CONFIG_LOG_ALWAYS_RUNTIME are enabled.

By default CONFIG_ASSERT is disabled unless the user has explicitly selected CONFIG_TEST (to indicate a test suite build). This means that asserts are not accidentally enabled. They have to be explicitly selected, same as all of the other features that impact code size and performance (e.g. use of logging, use of dynamic allocation, enabling multithreading, etc).

There's nothing unique about asserts that justifies a special warning. It's a feature that comes with costs, same as the rest.

benediktibk commented 4 days ago

There's nothing unique about asserts that justifies a special warning. It's a feature that comes with costs, same as the rest.

One could make the same argument for CONFIG_LOG, which similarly to CONFIG_ASSERT increases the footprint systemwide, spread over all drivers and subsystems. And there is also no warning for enabling it, obviously.

nashif commented 4 days ago

The analogy between CONFIG_LOG and CONFIG_ASSERT is not a good one if we step back and consider the original purpose of CONFIG_ASSERT. CONFIG_ASSERT is not supposed to be used in production, CONFIG_LOG is. Now the main problem and issue we have is the fact that CONFIG_ASSERT is being misused in countless places for runtime error checking and not enabling it might lead to undefined behaviour because of a lack of any other error checking in some critical areas.

Removing the watning makes this a defacto error checking mechanism and because of the mixed usage of CONFIG_ASSERT (usage as a debugging tool that has some penalties in critical area like the kernel and as a general error checking option) it will have negative impact.

So before we go and make this a fact, I would like us to review ASSERT and error checking and evenetually eliminate the mis-use of ASSERT and go toward runtime error checking or use the CHECKIF macros which were introduced to deal with this problem some time ago.

benediktibk commented 4 days ago

Okay, @nashif forget the analogy with CONFIG_LOG. But what for instance about CONFIG_DEBUG, like @palchak-google mentioned?

nashif commented 4 days ago

Okay, @nashif forget the analogy with CONFIG_LOG. But what for instance about CONFIG_DEBUG, like @palchak-google mentioned?

yeah, probably we need to add a similar warning for CONFIG_DEBUG.

benediktibk commented 4 days ago

Okay, @nashif forget the analogy with CONFIG_LOG. But what for instance about CONFIG_DEBUG, like @palchak-google mentioned?

yeah, probably we need to add a similar warning for CONFIG_DEBUG.

This would make it then at least consistent, but I am still not convinced of the necessity for a warning. Also a pure debug build should be in my opinion error and warning free. This is at least what I am aiming for. Having warnings which are triggered for features, which I intentionally activate, are a blocker for letting a build fail in CI for warnings, like -Werror -Wall. Which in the end introduces the possibility of other warnings, which are indisputed valid.

nashif commented 3 days ago

just noticed this https://github.com/zephyrproject-rtos/zephyr/issues/67637, shows how ASSERTS are being misused for error checking, this is really bad...