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.
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 <[email protected]>
Signed-off-by: Nathan Chancellor <[email protected]>
---
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
On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
<[email protected]> 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 <[email protected]>
(Note: this is the change I suggested, so not sure whether Acked-by or
Reviewed-by is more appropriate).
>
> 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 <[email protected]>
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> 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
On Tue, 09 Oct 2018, Nick Desaulniers <[email protected]> wrote:
> On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
> <[email protected]> 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 <[email protected]>
> (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.
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 <[email protected]>
>> Signed-off-by: Nathan Chancellor <[email protected]>
>> ---
>> 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
On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
<[email protected]> wrote:
> On Tue, 09 Oct 2018, Nick Desaulniers <[email protected]> wrote:
>> On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
>> <[email protected]> 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 <[email protected]>
>> (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 <[email protected]>
>>> Signed-off-by: Nathan Chancellor <[email protected]>
>>> ---
>>> 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
>>>
On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
<[email protected]> wrote:
>
> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
> <[email protected]> wrote:
>
> > On Tue, 09 Oct 2018, Nick Desaulniers <[email protected]> wrote:
> >> On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
> >> <[email protected]> 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 <[email protected]>
> >> (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 <[email protected]>
> >>> Signed-off-by: Nathan Chancellor <[email protected]>
> >>> ---
> >>> 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
On Wed, 10 Oct 2018, Nick Desaulniers <[email protected]> wrote:
> On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
> <[email protected]> wrote:
>>
>> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
>> <[email protected]> wrote:
>>
>> > On Tue, 09 Oct 2018, Nick Desaulniers <[email protected]> wrote:
>> >> On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
>> >> <[email protected]> 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 <[email protected]>
>> >> (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?
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 <[email protected]>
>> >>> Signed-off-by: Nathan Chancellor <[email protected]>
>> >>> ---
>> >>> 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
On Wed, Oct 10, 2018 at 11:21 PM Jani Nikula
<[email protected]> wrote:
>
> On Wed, 10 Oct 2018, Nick Desaulniers <[email protected]> wrote:
> > On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
> > <[email protected]> wrote:
> >>
> >> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
> >> <[email protected]> wrote:
> >>
> >> > On Tue, 09 Oct 2018, Nick Desaulniers <[email protected]> wrote:
> >> >> On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
> >> >> <[email protected]> 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 <[email protected]>
> >> >> (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 <[email protected]>
> >> >>> Signed-off-by: Nathan Chancellor <[email protected]>
> >> >>> ---
> >> >>> 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