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

[Nick Desaulniers]

On Wed, Oct 10, 2018 at 11:21 PM Jani Nikula
<jani.nikula@xxxxxxxxxxxxxxx> wrote:
>
> On Wed, 10 Oct 2018, Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote:
> > On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
> > <michal.wajdeczko@xxxxxxxxx> wrote:
> >>
> >> 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.
> >
> > I was also thinking of this.
>
> So does this fix the issue?

Yes, that should do the trick.  I assume this macro can also be
rewritten to use __builtin_types_compatible_p and
__builtin_choose_expr (rather than tokenizing the type and using
__builtin_strcmp), but maybe an exercise for another day.  We're happy
with the simplest fix acceptable for now.

>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index bd6bd8879cab..8d71886b5f03 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -184,7 +184,8 @@ static __always_inline void _print_param(struct drm_printer *p,
>         else if (!__builtin_strcmp(type, "char *"))
>                 drm_printf(p, "i915.%s=%s\n", name, *(const char **)x);
>         else
> -               BUILD_BUG();
> +               WARN_ONCE(1, "no printer defined for param type %s (i915.%s)\n",
> +                         type, name);
>  }
>
>  /**
>
> ---
>
> >
> >>
> >> Option 2: Use aliases instead of real types in param() macros.
> >
> > Will that affect other users of I915_PARAMS_FOR_EACH than _print_param?
> >
> > Either way, thanks for the help towards resolving this! We appreciate it!
> >
> >>
> >> 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/
>
> I can't find this on the list; was this sent just to patchwork or what?
>
> BR,
> Jani.
>
>
> >>
> >>
> >> >
> >> > 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
> >> >>>
>
> --
> Jani Nikula, Intel Open Source Graphics Center



-- 
Thanks,
~Nick Desaulniers