On Fri, Aug 31, 2018 at 12:35 AM Igor Stoppa <[email protected]> wrote:
>
> In some cases, checks that are expected to not fail, do not take advantage
> of specifying that the condition being tested is unlikely().
>
> Ex:
>
> #define assert(condition)
> ...
> if (!(condition))
> error_action()
> ...
>
> should be
>
> #define assert(condition)
> ...
> if (unlikely(!(condition)))
> error_action()
> ...
There is a potential that this introduces false-postive -Wmaybe-uninitialized
warnings when CONFIG_PROFILE_ANNOTATED_BRANCHES is
set, since that turns unlikely() into a complex operation that in turn
confuses the compiler so it no longer keeps track of which variables
are initialized or not.
It's possible that none of your patches do that, but one needs to be aware
of the problem, and possibly revert some of your patches if it does cause
warning regressions in drivers that don't actually benefit from the
micro-optimization.
> In other cases, the hint is given twice: once explicitly and once inside
> whatever test is being performed: assert(), BUG_ON(), WARN_ON(), etc.
>
> Ex:
>
> if (unlikely(WARN_ON(condition, message)))
> ...
Removing those is definitely fine.
Arnd
On 31/08/18 17:09, Arnd Bergmann wrote:
[...]
>> #define assert(condition)
>> ...
>> if (unlikely(!(condition)))
>> error_action()
>> ...
>
> There is a potential that this introduces false-postive -Wmaybe-uninitialized
> warnings when CONFIG_PROFILE_ANNOTATED_BRANCHES is
> set, since that turns unlikely() into a complex operation that in turn
> confuses the compiler so it no longer keeps track of which variables
> are initialized or not.
>
> It's possible that none of your patches do that, but one needs to be aware
> of the problem, and possibly revert some of your patches if it does cause
> warning regressions in drivers that don't actually benefit from the
> micro-optimization.
I see.
But if such case might arise, it might be possible, instead of reverting
these patches, to locally - and conditionally - turn unlikely() into an
identity macro.
For example:
#ifdef CONFIG_PROFILE_ANNOTATOED_BRANCHES
#define unlikely(x) (x)
#endif
Without giving up the hint for the case of normal compilation.
--
igor