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. > > 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@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 > >>> -- Thanks, ~Nick Desaulniers _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
