Messages in this thread Patch in this message | | | Subject | Re: [Intel-gfx] [PATCH] drm/i915: mark expected switch fall-through | From | "Gustavo A. R. Silva" <> | Date | Tue, 26 Jun 2018 19:43:16 -0500 |
| |
Hi Jani,
On 06/21/2018 03:03 AM, Jani Nikula wrote: > On Wed, 20 Jun 2018, "Gustavo A. R. Silva" <gustavo@embeddedor.com> 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:
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
| |