| Message ID | 357b53aa-d8ce-c9db-0b81-2e8e1aa821bb@embeddedor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 26 Jun 2018, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote: > 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: 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
> > 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. > Yep. > *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. > OK. I'll send a patch for the whole i915 subsystem, shortly. Thanks for the feedback. -- Gustavo
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) \