Re: [Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro

[Michal Wajdeczko]

On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula  
<jani.nikula@xxxxxxxxxxxxxxx> wrote:

> On Tue, 09 Oct 2018, Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote:
> > On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
> > <natechancellor@xxxxxxxxx> wrote:
> > > When building the kernel with Clang with defconfig and CONFIG_64BIT
> > > disabled, vmlinux fails to link because of the BUILD_BUG in
> > > _print_param.
> > >
> > > ld: drivers/gpu/drm/i915/i915_params.o: in function `i915_params_dump':
> > > i915_params.c:(.text+0x56): undefined reference to
> > > `__compiletime_assert_191'
> > >
> > > This function is semantically invalid unless the code is first inlined
> > > then constant folded, which doesn't work for Clang because semantic
> > > analysis happens before optimization/inlining. Converting this function
> > > to a macro avoids this problem and allows Clang to properly remove the
> > > BUILD_BUG during optimization.
> >
> > Thanks Nathan for the patch.  To provide more context, Clang does
> > semantic analysis before optimization, where as GCC does these
> > together (IIUC).  So the above link error is from the naked
> > BUILD_BUG().  Clang can't evaluate the __builtin_strcmp's statically
> > until inlining has occurred, but that optimization happens after
> > semantic analysis.  To do the inlining before semantic analysis, we
> > MUST leverage the preprocessor, which runs before the compiler starts
> > doing semantic analysis.  I suspect this code is not valid for GCC
> > unless optimizations are enabled (the kernel only does compile with
> > optimizations turned on).  This change allows us to build this
> > translation unit with Clang.
> >
> > Acked-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> > (Note: this is the change I suggested, so not sure whether Acked-by or
> > Reviewed-by is more appropriate).
>
> *Sad trombone*
>
> I'd rather see us converting more macros to static inlines than the
> other way round.
>
> I'll let others chime in if they have any better ideas, otherwise I'll
> apply this one.

Option 1: Just drop BUILD_BUG() from _print_param() function.

Option 2: Use aliases instead of real types in param() macros.

Aliases can be same as in linux/moduleparam.h (charp|int|uint|bool)
We can convert aliases back to real types but it will also allow
to construct proper names for dedicated functions - see [1]

Michal

[1] https://patchwork.freedesktop.org/patch/255928/


> BR,
> Jani.
>
> > > The output of 'objdump -D' is identically before and after this change
> > > for GCC regardless of if CONFIG_64BIT is set and allows Clang to link
> > > the kernel successfully with or without CONFIG_64BIT set.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/191
> > > Suggested-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> > > Signed-off-by: Nathan Chancellor <natechancellor@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/i915_params.c | 29 +++++++++++++----------------
> > >  1 file changed, 13 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_params.c  
> > > b/drivers/gpu/drm/i915/i915_params.c
> > > index 295e981e4a39..a0f20b9b6f2d 100644
> > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > @@ -174,22 +174,19 @@ i915_param_named(enable_dpcd_backlight, bool,  
> > > 0600,
> > >  i915_param_named(enable_gvt, bool, 0400,
> > >         "Enable support for Intel GVT-g graphics virtualization host  
> > > support(default:false)");
> > >
> > > -static __always_inline void _print_param(struct drm_printer *p,
> > > -                                        const char *name,
> > > -                                        const char *type,
> > > -                                        const void *x)
> > > -{
> > > -       if (!__builtin_strcmp(type, "bool"))
> > > -               drm_printf(p, "i915.%s=%s\n", name, yesno(*(const bool  
> > > *)x));
> > > -       else if (!__builtin_strcmp(type, "int"))
> > > -               drm_printf(p, "i915.%s=%d\n", name, *(const int *)x);
> > > -       else if (!__builtin_strcmp(type, "unsigned int"))
> > > -               drm_printf(p, "i915.%s=%u\n", name, *(const unsigned  
> > > int *)x);
> > > -       else if (!__builtin_strcmp(type, "char *"))
> > > -               drm_printf(p, "i915.%s=%s\n", name, *(const char **)x);
> > > -       else
> > > -               BUILD_BUG();
> > > -}
> > > +#define _print_param(p, name, type,  
> > > x)                                        \
> > > +do  
> > > {                                                                           
> > > \
> > > +       if (!__builtin_strcmp(type,  
> > > "bool"))                                   \
> > > +               drm_printf(p, "i915.%s=%s\n", name, yesno(*(const bool  
> > > *)x));  \
> > > +       else if (!__builtin_strcmp(type,  
> > > "int"))                               \
> > > +               drm_printf(p, "i915.%s=%d\n", name, *(const int  
> > > *)x);          \
> > > +       else if (!__builtin_strcmp(type, "unsigned  
> > > int"))                      \
> > > +               drm_printf(p, "i915.%s=%u\n", name, *(const unsigned  
> > > int *)x); \
> > > +       else if (!__builtin_strcmp(type, "char  
> > > *"))                            \
> > > +               drm_printf(p, "i915.%s=%s\n", name, *(const char  
> > > **)x);        \
> > > +        
> > > else                                                                    
> > > \
> > > +                
> > > BUILD_BUG();                                                   \
> > > +} while (0)
> > >
> > >  /**
> > >   * i915_params_dump - dump i915 modparams
> > > --
> > > 2.19.0