| Message ID | 20180313000312.24268-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Lucas De Marchi (2018-03-13 00:03:12) > In some places we end up converting switch statements to a series of > if/else, particularly when introducing helper functions to handle a > group of cases. It's tempting to either leave a wrong warning (since now > we don't have a switch case anymore) or to convert to WARN(1, ...), > losing what MISSING_CASE() provides: source location and id number. > > Fix the message to allow reusing MISSING_CASE() - it may not always be > correct (e.g. if you are not checking an id anymore), but it avoids > useless conversions. A quick grep reveals at least a few users in > drivers/gpu/drm/i915/intel_csr.c and drivers/gpu/drm/i915/intel_ddi.c. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/i915_utils.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > index 51dbfe5bb418..8cdc21b92f5f 100644 > --- a/drivers/gpu/drm/i915/i915_utils.h > +++ b/drivers/gpu/drm/i915/i915_utils.h > @@ -40,7 +40,7 @@ > #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 switch case (%lu) in %s\n", \ > +#define MISSING_CASE(x) WARN(1, "Missing case (%lu) in %s\n", \ > (long)(x), __func__) Whilst here you could make this more informative by: "Missing case (%s = %lu) in %s\n", __stringify(x), (long)(x), __func__ Also these should only be compiled for dev/CI as we have a few pages worth of them. -Chris
On Tue, Mar 13, 2018 at 12:17:13AM +0000, Chris Wilson wrote: > Quoting Lucas De Marchi (2018-03-13 00:03:12) > > In some places we end up converting switch statements to a series of > > if/else, particularly when introducing helper functions to handle a > > group of cases. It's tempting to either leave a wrong warning (since now > > we don't have a switch case anymore) or to convert to WARN(1, ...), > > losing what MISSING_CASE() provides: source location and id number. > > > > Fix the message to allow reusing MISSING_CASE() - it may not always be > > correct (e.g. if you are not checking an id anymore), but it avoids > > useless conversions. A quick grep reveals at least a few users in > > drivers/gpu/drm/i915/intel_csr.c and drivers/gpu/drm/i915/intel_ddi.c. > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_utils.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > > index 51dbfe5bb418..8cdc21b92f5f 100644 > > --- a/drivers/gpu/drm/i915/i915_utils.h > > +++ b/drivers/gpu/drm/i915/i915_utils.h > > @@ -40,7 +40,7 @@ > > #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 switch case (%lu) in %s\n", \ > > +#define MISSING_CASE(x) WARN(1, "Missing case (%lu) in %s\n", \ > > (long)(x), __func__) > > Whilst here you could make this more informative by: > "Missing case (%s = %lu) in %s\n", __stringify(x), (long)(x), __func__ The backtrace isn't enough? > > Also these should only be compiled for dev/CI as we have a few pages > worth of them. > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Ville Syrjälä (2018-03-13 13:01:42) > On Tue, Mar 13, 2018 at 12:17:13AM +0000, Chris Wilson wrote: > > Quoting Lucas De Marchi (2018-03-13 00:03:12) > > > In some places we end up converting switch statements to a series of > > > if/else, particularly when introducing helper functions to handle a > > > group of cases. It's tempting to either leave a wrong warning (since now > > > we don't have a switch case anymore) or to convert to WARN(1, ...), > > > losing what MISSING_CASE() provides: source location and id number. > > > > > > Fix the message to allow reusing MISSING_CASE() - it may not always be > > > correct (e.g. if you are not checking an id anymore), but it avoids > > > useless conversions. A quick grep reveals at least a few users in > > > drivers/gpu/drm/i915/intel_csr.c and drivers/gpu/drm/i915/intel_ddi.c. > > > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_utils.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > > > index 51dbfe5bb418..8cdc21b92f5f 100644 > > > --- a/drivers/gpu/drm/i915/i915_utils.h > > > +++ b/drivers/gpu/drm/i915/i915_utils.h > > > @@ -40,7 +40,7 @@ > > > #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 switch case (%lu) in %s\n", \ > > > +#define MISSING_CASE(x) WARN(1, "Missing case (%lu) in %s\n", \ > > > (long)(x), __func__) > > > > Whilst here you could make this more informative by: > > "Missing case (%s = %lu) in %s\n", __stringify(x), (long)(x), __func__ > > The backtrace isn't enough? Not if we have more than one in a function. (Why would we do that, you might ask, and I'd answer if the point is to make this more generic and versatile, then do so. We already have the value, why not then explain what that value is.) And give me a single sentence identifying the missing case makes it much more pleasant. -Chris
On Tue, Mar 13, 2018 at 01:06:49PM +0000, Chris Wilson wrote: > Quoting Ville Syrjälä (2018-03-13 13:01:42) > > On Tue, Mar 13, 2018 at 12:17:13AM +0000, Chris Wilson wrote: > > > Quoting Lucas De Marchi (2018-03-13 00:03:12) > > > > In some places we end up converting switch statements to a series of > > > > if/else, particularly when introducing helper functions to handle a > > > > group of cases. It's tempting to either leave a wrong warning (since now > > > > we don't have a switch case anymore) or to convert to WARN(1, ...), > > > > losing what MISSING_CASE() provides: source location and id number. > > > > > > > > Fix the message to allow reusing MISSING_CASE() - it may not always be > > > > correct (e.g. if you are not checking an id anymore), but it avoids > > > > useless conversions. A quick grep reveals at least a few users in > > > > drivers/gpu/drm/i915/intel_csr.c and drivers/gpu/drm/i915/intel_ddi.c. > > > > > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_utils.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > > > > index 51dbfe5bb418..8cdc21b92f5f 100644 > > > > --- a/drivers/gpu/drm/i915/i915_utils.h > > > > +++ b/drivers/gpu/drm/i915/i915_utils.h > > > > @@ -40,7 +40,7 @@ > > > > #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 switch case (%lu) in %s\n", \ > > > > +#define MISSING_CASE(x) WARN(1, "Missing case (%lu) in %s\n", \ > > > > (long)(x), __func__) > > > > > > Whilst here you could make this more informative by: > > > "Missing case (%s = %lu) in %s\n", __stringify(x), (long)(x), __func__ > > > > The backtrace isn't enough? > > Not if we have more than one in a function. I was just commenting on the __func__ part actually. That seems pretty much redundant to me. The stringify part does seem like a decent idea, and matches our WARN() trickery pretty well. > (Why would we do that, you > might ask, and I'd answer if the point is to make this more generic and > versatile, then do so. We already have the value, why not then explain > what that value is.) And give me a single sentence identifying the missing > case makes it much more pleasant. > -Chris
On Tue, Mar 13, 2018 at 03:23:54PM +0200, Ville Syrjälä wrote: > On Tue, Mar 13, 2018 at 01:06:49PM +0000, Chris Wilson wrote: > > Quoting Ville Syrjälä (2018-03-13 13:01:42) > > > On Tue, Mar 13, 2018 at 12:17:13AM +0000, Chris Wilson wrote: > > > > Quoting Lucas De Marchi (2018-03-13 00:03:12) > > > > > In some places we end up converting switch statements to a series of > > > > > if/else, particularly when introducing helper functions to handle a > > > > > group of cases. It's tempting to either leave a wrong warning (since now > > > > > we don't have a switch case anymore) or to convert to WARN(1, ...), > > > > > losing what MISSING_CASE() provides: source location and id number. > > > > > > > > > > Fix the message to allow reusing MISSING_CASE() - it may not always be > > > > > correct (e.g. if you are not checking an id anymore), but it avoids > > > > > useless conversions. A quick grep reveals at least a few users in > > > > > drivers/gpu/drm/i915/intel_csr.c and drivers/gpu/drm/i915/intel_ddi.c. > > > > > > > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_utils.h | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > > > > > index 51dbfe5bb418..8cdc21b92f5f 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_utils.h > > > > > +++ b/drivers/gpu/drm/i915/i915_utils.h > > > > > @@ -40,7 +40,7 @@ > > > > > #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 switch case (%lu) in %s\n", \ > > > > > +#define MISSING_CASE(x) WARN(1, "Missing case (%lu) in %s\n", \ > > > > > (long)(x), __func__) > > > > > > > > Whilst here you could make this more informative by: > > > > "Missing case (%s = %lu) in %s\n", __stringify(x), (long)(x), __func__ > > > > > > The backtrace isn't enough? > > > > Not if we have more than one in a function. > > I was just commenting on the __func__ part actually. That seems pretty > much redundant to me. The stringify part does seem like a decent idea, > and matches our WARN() trickery pretty well. Humn... indeed. On a quick look I thought it was there so we wouldn't need a addr2line. But we are not even printing the line number, just the function name. The warning is from the kernel already does that and more. What about: #define MISSING_CASE(x) WARN(1, "Missing case (%s == %lu)\n", \ __stringify(x), (unsigned long)(x)) I added a simple hack on probe and I get this: [ 4535.233717] Missing case (ret == 0) [ 4535.233868] WARNING: CPU: 1 PID: 795 at drivers/gpu/drm/i915/i915_drv.c:1341 i915_driver_load+0x42/0x10e0 [i915] Lucas De Marchi > > > (Why would we do that, you > > might ask, and I'd answer if the point is to make this more generic and > > versatile, then do so. We already have the value, why not then explain > > what that value is.) And give me a single sentence identifying the missing > > case makes it much more pleasant. > > -Chris > > -- > Ville Syrjälä > Intel OTC
CCing checkpatch maintainers On Mon, Mar 19, 2018 at 06:36:16PM +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Reword warning for missing cases (rev2) > URL : https://patchwork.freedesktop.org/series/39821/ > State : warning > > == Summary == > > $ dim checkpatch origin/drm-tip > 7cb94d2c3c47 drm/i915: Reword warning for missing cases > -:40: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'x' - possible side-effects? > #40: FILE: drivers/gpu/drm/i915/i915_utils.h:43: > +#define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \ > + __stringify(x), (long)(x)) checkpatch seems wrong here since __stringify() doesn't evaluate x. And it looks similar to the one fixed by e942e2c3f7e0 ("checkpatch: fix stringification macro defect") Lucas De Marchi > > total: 0 errors, 0 warnings, 1 checks, 10 lines checked >
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h index 51dbfe5bb418..8cdc21b92f5f 100644 --- a/drivers/gpu/drm/i915/i915_utils.h +++ b/drivers/gpu/drm/i915/i915_utils.h @@ -40,7 +40,7 @@ #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 switch case (%lu) in %s\n", \ +#define MISSING_CASE(x) WARN(1, "Missing case (%lu) in %s\n", \ (long)(x), __func__) #if GCC_VERSION >= 70000
In some places we end up converting switch statements to a series of if/else, particularly when introducing helper functions to handle a group of cases. It's tempting to either leave a wrong warning (since now we don't have a switch case anymore) or to convert to WARN(1, ...), losing what MISSING_CASE() provides: source location and id number. Fix the message to allow reusing MISSING_CASE() - it may not always be correct (e.g. if you are not checking an id anymore), but it avoids useless conversions. A quick grep reveals at least a few users in drivers/gpu/drm/i915/intel_csr.c and drivers/gpu/drm/i915/intel_ddi.c. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/i915_utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)