lkml.org 
[lkml]   [2018]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro
Date
From
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.

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@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
>>>

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