Re: [Intel-gfx] [PATCH] drm/i915: mark expected switch fall-through

[Jani Nikula]

On Tue, 26 Jun 2018, "Gustavo A. R. Silva" <gustavo@xxxxxxxxxxxxxx> wrote:
> Hi Jani,
>
> On 06/21/2018 03:03 AM, Jani Nikula wrote:
>> On Wed, 20 Jun 2018, "Gustavo A. R. Silva" <gustavo@xxxxxxxxxxxxxx> wrote:
>>> On 06/20/2018 02:06 PM, Rodrigo Vivi wrote:
>>>> On Wed, Jun 20, 2018 at 08:31:00AM -0500, Gustavo A. R. Silva wrote:
>>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>>>> where we are expecting to fall through.
>>>>>
>>>>> Addresses-Coverity-ID: 1470102 ("Missing break in switch")
>>>>
>>>> Any other advantage besides coverity?
>>>> Can't we address it by marking as "Intentional" on the tool?
>>>>
>>>
>>> Yes. The advantage of this is that it will eventually allows to enable 
>>> -Wimplicit-fallthrough, hence, enabling the compiler to trigger a 
>>> warning, which will force us to double check if we are actually missing 
>>> a break before committing the code.
>> 
>> I applaud the efforts. Since you're doing the comment changes, do you
>> have an idea what -Wimplicit-fallthrough=N level is being considered for
>> kernel?
>> 
>
> Currently, we are trying level 2.
>
>>>> I'm afraid there will be so many more places to add fallthrough
>>>> marks....
>>>>
>>>
>>> Oh yeah, there are around 1000 similar places in the whole codebase. 
>>> There is an ongoing effort to review each case. Months ago, it used to 
>>> be around 1500 of these cases.
>> 
>> We use our own MISSING_CASE() to indicate stuff that's not supposed to
>> happen, or to be implemented, etc. and in many cases the fallthrough is
>> normal. I wonder if we could embed __attribute__ ((fallthrough)) in
>> there to tackle all of these without a comment.
>> 
>
> I've tried this:
>
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index 00165ad..829572c 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -40,8 +40,10 @@
>  #undef WARN_ON_ONCE
>  #define WARN_ON_ONCE(x) WARN_ONCE((x), "%s", "WARN_ON_ONCE(" __stringify(x) ")")
>
> -#define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
> -                            __stringify(x), (long)(x))
> +#define MISSING_CASE(x) ({ \
> +       WARN(1, "Missing case (%s == %ld)\n", __stringify(x), (long)(x)); \
> +       __attribute__ ((fallthrough)); \
> +})
>
>  #if GCC_VERSION >= 70000
>  #define add_overflows(A, B) \
>
> and I get the following warnings as a consequence:

Right. That's because we've used MISSING_CASE() also in if-ladders in
addition to the switch default case. From our POV the usage is similar.

*shrug*

I guess I like /* fall through */ annotations next to MISSING_CASE()
better than having two different macros depending on where they're being
used.

Thanks for trying it out anyway.

BR,
Jani.


>
> drivers/gpu/drm/i915/intel_pm.c: In function âintel_init_clock_gating_hooksâ:
> drivers/gpu/drm/i915/i915_utils.h:48:2: error: invalid use of attribute âfallthroughâ
>   __attribute__ ((fallthrough)); \
>   ^
> drivers/gpu/drm/i915/intel_pm.c:9240:3: note: in expansion of macro âMISSING_CASEâ
>    MISSING_CASE(INTEL_DEVID(dev_priv));
>    ^~~~~~~~~~~~
> drivers/gpu/drm/i915/intel_pm.c: In function âintel_read_wm_latencyâ:
> drivers/gpu/drm/i915/i915_utils.h:48:2: error: invalid use of attribute âfallthroughâ
>   __attribute__ ((fallthrough)); \
>   ^
> drivers/gpu/drm/i915/intel_pm.c:2902:3: note: in expansion of macro âMISSING_CASEâ
>    MISSING_CASE(INTEL_DEVID(dev_priv));
>    ^~~~~~~~~~~~
>
> Thanks
> --
> Gustavo

-- 
Jani Nikula, Intel Open Source Graphics Center