lkml.org 
[lkml]   [2018]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro
On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>
> > On Tue, 09 Oct 2018, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >> On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
> >> <natechancellor@gmail.com> 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@google.com>
> >> (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.

>
> 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/
>
>
> >
> > 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@google.com>
> >>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> >>> ---
> >>> 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
> >>>



--
Thanks,
~Nick Desaulniers

\
 
 \ /
  Last update: 2018-10-10 22:42    [W:0.062 / U:16.020 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site