| Message ID | 20241029084859.135172-1-gye976@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/xe: Fix build error for XE_IOCTL_DBG macro | expand |
On Tue, Oct 29, 2024 at 05:48:58PM +0900, Gyeyoung Baek wrote: >if CONFIG_DRM_USE_DYNAMIC_DEBUG is set, >'drm_dbg' function is replaced with '__dynamic_func_call_cls', >which is replaced with a do while statement. >so in the previous code, there are the following build errors. > >include/linux/dynamic_debug.h:221:58: error: expected expression before ‘do’ > 221 | #define __dynamic_func_call_cls(id, cls, fmt, func, ...) do { \ > | ^~ >include/linux/dynamic_debug.h:248:9: note: in expansion of macro ‘__dynamic_func_call_cls’ > 248 | __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__) > | ^~~~~~~~~~~~~~~~~~~~~~~ >include/drm/drm_print.h:425:9: note: in expansion of macro ‘_dynamic_func_call_cls’ > 425 | _dynamic_func_call_cls(cat, fmt, __drm_dev_dbg, \ > | ^~~~~~~~~~~~~~~~~~~~~~ >include/drm/drm_print.h:504:9: note: in expansion of macro ‘drm_dev_dbg’ > 504 | drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__) > | ^~~~~~~~~~~ >include/drm/drm_print.h:522:33: note: in expansion of macro ‘drm_dbg_driver’ > 522 | #define drm_dbg(drm, fmt, ...) drm_dbg_driver(drm, fmt, ##__VA_ARGS__) > | ^~~~~~~~~~~~~~ >drivers/gpu/drm/xe/xe_macros.h:14:21: note: in expansion of macro ‘drm_dbg’ > 14 | ((cond) && (drm_dbg(&(xe)->drm, \ > | ^~~~~~~ >drivers/gpu/drm/xe/xe_bo.c:2029:13: note: in expansion of macro ‘XE_IOCTL_DBG’ > 2029 | if (XE_IOCTL_DBG(xe, !gem_obj)) > >the problem is that, >XE_IOCTL_DBG uses this function for conditional expr. > >so I fix the expr to be compatible with the do while statement, >by referring to "https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html". > >v2: I modified this to print when only cond is true. > >Signed-off-by: Gyeyoung Baek <gye976@gmail.com> >--- > drivers/gpu/drm/xe/xe_macros.h | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > >diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h >index daf56c846d03..ac2bd103bb22 100644 >--- a/drivers/gpu/drm/xe/xe_macros.h >+++ b/drivers/gpu/drm/xe/xe_macros.h >@@ -11,8 +11,12 @@ > #define XE_WARN_ON WARN_ON > > #define XE_IOCTL_DBG(xe, cond) \ >- ((cond) && (drm_dbg(&(xe)->drm, \ >- "Ioctl argument check failed at %s:%d: %s", \ >- __FILE__, __LINE__, #cond), 1)) >+({ \ >+ if ((cond)) \ >+ drm_dbg(&(xe)->drm, \ >+ "Ioctl argument check failed at %s:%d: %s", \ >+ __FILE__, __LINE__, #cond); \ >+ (cond); \ there's a double cond evaluation here and given any expression can be given to XE_IOCTL_DBG(), this doens't look very safe. I think this would be safer as: #define XE_IOCTL_DBG(xe, cond) ({ \ int cond__ = !!(cond); \ if (cond__) \ drm_dbg(&(xe)->drm, \ "Ioctl argument check failed at %s:%d: %s", \ __FILE__, __LINE__, #cond); \ cond__; \ }) as it then evaluates cond just once. Also the generated code seems to be sane compared to what we had before too. And I also needed this to build-test: | diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c | index 08cfea04e22bd..82585d442f017 100644 | --- a/drivers/gpu/drm/drm_print.c | +++ b/drivers/gpu/drm/drm_print.c | @@ -215,9 +215,8 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf) | { | const struct drm_device *drm = p->arg; | const struct device *dev = drm ? drm->dev : NULL; | - enum drm_debug_category category = p->category; | | - if (!__drm_debug_enabled(category)) | + if (!__drm_debug_enabled(p->category)) | return; | | __drm_dev_vprintk(dev, KERN_DEBUG, p->origin, p->prefix, vaf); as otherwise it complains category is unused. Lucas De Marchi
> >--- a/drivers/gpu/drm/xe/xe_macros.h > >+++ b/drivers/gpu/drm/xe/xe_macros.h > >@@ -11,8 +11,12 @@ > > #define XE_WARN_ON WARN_ON > > > > #define XE_IOCTL_DBG(xe, cond) \ > >- ((cond) && (drm_dbg(&(xe)->drm, \ > >- "Ioctl argument check failed at %s:%d: %s", \ > >- __FILE__, __LINE__, #cond), 1)) > >+({ \ > >+ if ((cond)) \ > >+ drm_dbg(&(xe)->drm, \ > >+ "Ioctl argument check failed at %s:%d: %s", \ > >+ __FILE__, __LINE__, #cond); \ > >+ (cond); \ > > there's a double cond evaluation here and given any expression can be > given to XE_IOCTL_DBG(), this doens't look very safe. I think this would > be safer as: > > #define XE_IOCTL_DBG(xe, cond) ({ \ > int cond__ = !!(cond); \ > if (cond__) \ > drm_dbg(&(xe)->drm, \ > "Ioctl argument check failed at %s:%d: %s", \ > __FILE__, __LINE__, #cond); \ > cond__; \ > }) > > as it then evaluates cond just once. Also the generated code seems to be > sane compared to what we had before too. > Yes, if (cond) has operator like ++, it will be a bug. I miss that... I will revise a patch again by referring to your review, thanks. > And I also needed this to build-test: > > | diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > | index 08cfea04e22bd..82585d442f017 100644 > | --- a/drivers/gpu/drm/drm_print.c > | +++ b/drivers/gpu/drm/drm_print.c > | @@ -215,9 +215,8 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf) > | { > | const struct drm_device *drm = p->arg; > | const struct device *dev = drm ? drm->dev : NULL; > | - enum drm_debug_category category = p->category; > | > | - if (!__drm_debug_enabled(category)) > | + if (!__drm_debug_enabled(p->category)) > | return; > | > | __drm_dev_vprintk(dev, KERN_DEBUG, p->origin, p->prefix, vaf); > > as otherwise it complains category is unused. > I also submitted a seperate patch to fix '__drm_debug_enabled' macro, from '#define __drm_debug_enabled(category) true' to '#define __drm_debug_enabled(category) ({ void(category); true; })' This removes the build error caused by the unused 'category', too. Anyway, it can be build. I tested both cases. I realize now that these two patches should have been submitted as a patch series I'm sorry for my mistakes. Thanks, Gyeyoung Baek > Lucas De Marchi
diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h index daf56c846d03..ac2bd103bb22 100644 --- a/drivers/gpu/drm/xe/xe_macros.h +++ b/drivers/gpu/drm/xe/xe_macros.h @@ -11,8 +11,12 @@ #define XE_WARN_ON WARN_ON #define XE_IOCTL_DBG(xe, cond) \ - ((cond) && (drm_dbg(&(xe)->drm, \ - "Ioctl argument check failed at %s:%d: %s", \ - __FILE__, __LINE__, #cond), 1)) +({ \ + if ((cond)) \ + drm_dbg(&(xe)->drm, \ + "Ioctl argument check failed at %s:%d: %s", \ + __FILE__, __LINE__, #cond); \ + (cond); \ +}) #endif
if CONFIG_DRM_USE_DYNAMIC_DEBUG is set, 'drm_dbg' function is replaced with '__dynamic_func_call_cls', which is replaced with a do while statement. so in the previous code, there are the following build errors. include/linux/dynamic_debug.h:221:58: error: expected expression before ‘do’ 221 | #define __dynamic_func_call_cls(id, cls, fmt, func, ...) do { \ | ^~ include/linux/dynamic_debug.h:248:9: note: in expansion of macro ‘__dynamic_func_call_cls’ 248 | __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__) | ^~~~~~~~~~~~~~~~~~~~~~~ include/drm/drm_print.h:425:9: note: in expansion of macro ‘_dynamic_func_call_cls’ 425 | _dynamic_func_call_cls(cat, fmt, __drm_dev_dbg, \ | ^~~~~~~~~~~~~~~~~~~~~~ include/drm/drm_print.h:504:9: note: in expansion of macro ‘drm_dev_dbg’ 504 | drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__) | ^~~~~~~~~~~ include/drm/drm_print.h:522:33: note: in expansion of macro ‘drm_dbg_driver’ 522 | #define drm_dbg(drm, fmt, ...) drm_dbg_driver(drm, fmt, ##__VA_ARGS__) | ^~~~~~~~~~~~~~ drivers/gpu/drm/xe/xe_macros.h:14:21: note: in expansion of macro ‘drm_dbg’ 14 | ((cond) && (drm_dbg(&(xe)->drm, \ | ^~~~~~~ drivers/gpu/drm/xe/xe_bo.c:2029:13: note: in expansion of macro ‘XE_IOCTL_DBG’ 2029 | if (XE_IOCTL_DBG(xe, !gem_obj)) the problem is that, XE_IOCTL_DBG uses this function for conditional expr. so I fix the expr to be compatible with the do while statement, by referring to "https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html". v2: I modified this to print when only cond is true. Signed-off-by: Gyeyoung Baek <gye976@gmail.com> --- drivers/gpu/drm/xe/xe_macros.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)