| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87o9c1vwgc.fsf@intel.com>
Date: Thu, 11 Oct 2018 09:17:23 +0300
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Nick Desaulniers <ndesaulniers@...gle.com>,
michal.wajdeczko@...el.com
Cc: Nathan Chancellor <natechancellor@...il.com>,
intel-gfx@...ts.freedesktop.org,
LKML <linux-kernel@...r.kernel.org>,
dri-devel@...ts.freedesktop.org, rodrigo.vivi@...el.com,
lukas.bulwahn@...il.com
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro
On Wed, 10 Oct 2018, Nick Desaulniers <ndesaulniers@...gle.com> wrote:
> On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
> <michal.wajdeczko@...el.com> wrote:
>>
>> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
>> <jani.nikula@...ux.intel.com> wrote:
>>
>> > On Tue, 09 Oct 2018, Nick Desaulniers <ndesaulniers@...gle.com> wrote:
>> >> On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
>> >> <natechancellor@...il.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@...gle.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.
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 <ndesaulniers@...gle.com>
>> >>> Signed-off-by: Nathan Chancellor <natechancellor@...il.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
>> >>>
--
Jani Nikula, Intel Open Source Graphics Center
Powered by blists - more mailing lists