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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
